AlbumenJ commented on code in PR #11021: URL: https://github.com/apache/dubbo/pull/11021#discussion_r1080728528
########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/DubboClientContext.java: ########## @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import java.util.Objects; + +import io.micrometer.observation.transport.SenderContext; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.RpcContextAttachment; + +/** + * Provider context for RPC. + */ +public class DubboClientContext extends SenderContext<RpcContextAttachment> { + + private final RpcContextAttachment rpcContextAttachment; + + private final Invoker<?> invoker; + + private final Invocation invocation; + + public DubboClientContext(RpcContextAttachment rpcContextAttachment, Invoker<?> invoker, Invocation invocation) { + super((map, key, value) -> Objects.requireNonNull(map).setAttachment(key, value)); + this.rpcContextAttachment = rpcContextAttachment; + this.invoker = invoker; + this.invocation = invocation; + setCarrier(rpcContextAttachment); + } Review Comment: ```suggestion public DubboClientContext(Invoker<?> invoker, Invocation invocation) { super((map, key, value) -> Objects.requireNonNull(map).setAttachment(key, value)); this.invoker = invoker; this.invocation = invocation; setCarrier(invocation); } ``` Directly set attachments, which can be send to remote, by `invocation.setAttachment`. `RpcContextAttachment` is design as a user-mode API, not for framework itself. ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/AbstractDefaultDubboObservationConvention.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.common.KeyValues; +import io.micrometer.common.docs.KeyName; +import io.micrometer.common.lang.Nullable; +import org.apache.dubbo.common.utils.StringUtils; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.RpcContextAttachment; + +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.NET_PEER_NAME; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.NET_PEER_PORT; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_METHOD; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_SERVICE; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_SYSTEM; + +class AbstractDefaultDubboObservationConvention { + KeyValues getLowCardinalityKeyValues(Invocation invocation, RpcContextAttachment rpcContextAttachment) { + KeyValues keyValues = KeyValues.of(RPC_SYSTEM.withValue("apache_dubbo")); + String serviceName = StringUtils.hasText(invocation.getServiceName()) ? invocation.getServiceName() : readServiceName(invocation.getTargetServiceUniqueName()); + keyValues = appendNonNull(keyValues, RPC_SERVICE, serviceName); + keyValues = appendNonNull(keyValues, RPC_METHOD, invocation.getMethodName()); + keyValues = appendNonNull(keyValues, NET_PEER_NAME, rpcContextAttachment.getRemoteHostName()); + if (rpcContextAttachment.getRemotePort() == 0) { + return keyValues; + } + int port = rpcContextAttachment.getRemotePort() != 0 ? rpcContextAttachment.getRemotePort() : rpcContextAttachment.getLocalPort(); Review Comment: `NET_PEER_NAME` is design for provider side or consumer side? If for provider side, use `org.apache.dubbo.rpc.RpcContext#getServiceContext.getRemoteAddress()` instead. `RpcContextAttachment` is design for attachment only. The `getRemotePort` here is for compatible purpose. If for consumer side, it is not a good idea to track it because consumer can call for retry or boardcast. Thus, it is hard to fetch the actual remote address, but it is possible. :) ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/ObservationSenderFilter.java: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import org.apache.dubbo.common.extension.Activate; +import org.apache.dubbo.rpc.BaseFilter; +import org.apache.dubbo.rpc.Filter; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.Result; +import org.apache.dubbo.rpc.RpcContext; +import org.apache.dubbo.rpc.RpcContextAttachment; +import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.cluster.filter.ClusterFilter; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ScopeModelAware; + +import static org.apache.dubbo.common.constants.CommonConstants.CONSUMER; + +/** + * A {@link Filter} that creates an {@link Observation} around the outgoing message. + */ +@Activate(group = CONSUMER, order = -1) +public class ObservationSenderFilter implements ClusterFilter, BaseFilter.Listener, ScopeModelAware { + + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private DubboClientObservationConvention clientObservationConvention = null; + + @Override + public void setApplicationModel(ApplicationModel applicationModel) { + observationRegistry = applicationModel.getBeanFactory().getBean(ObservationRegistry.class); + clientObservationConvention = applicationModel.getBeanFactory().getBean(DubboClientObservationConvention.class); + } Review Comment: ```suggestion public ObservationSenderFilter(ApplicationModel applicationModel) { observationRegistry = applicationModel.getBeanFactory().getBean(ObservationRegistry.class); clientObservationConvention = applicationModel.getBeanFactory().getBean(DubboClientObservationConvention.class); } ``` Use constructor instead is better, which can prevent NPE in initial. BTW, as for test, you can use `ApplicationModel.defaultModel()` for test purpose. `defaultModel` is only for test or compatible purpose and should be prevented to being used for production code. ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/ObservationReceiverFilter.java: ########## @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import org.apache.dubbo.common.extension.Activate; +import org.apache.dubbo.rpc.BaseFilter; +import org.apache.dubbo.rpc.Filter; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.Result; +import org.apache.dubbo.rpc.RpcContext; +import org.apache.dubbo.rpc.RpcContextAttachment; +import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ScopeModelAware; + +import static org.apache.dubbo.common.constants.CommonConstants.PROVIDER; + +/** + * A {@link Filter} that creates an {@link Observation} around the incoming message. + */ +@Activate(group = PROVIDER, order = -1) +public class ObservationReceiverFilter implements Filter, BaseFilter.Listener, ScopeModelAware { + + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private DubboServerObservationConvention serverObservationConvention = null; + + @Override + public void setApplicationModel(ApplicationModel applicationModel) { + observationRegistry = applicationModel.getBeanFactory().getBean(ObservationRegistry.class); + serverObservationConvention = applicationModel.getBeanFactory().getBean(DubboServerObservationConvention.class); + } + + @Override + public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException { + if (observationRegistry == null) { + return invoker.invoke(invocation); + } + RpcContextAttachment context = RpcContext.getServerAttachment(); + DubboServerContext receiverContext = new DubboServerContext(context, invoker, invocation); Review Comment: Use invocation instead. ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/ObservationReceiverFilter.java: ########## @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import org.apache.dubbo.common.extension.Activate; +import org.apache.dubbo.rpc.BaseFilter; +import org.apache.dubbo.rpc.Filter; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.Result; +import org.apache.dubbo.rpc.RpcContext; +import org.apache.dubbo.rpc.RpcContextAttachment; +import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ScopeModelAware; + +import static org.apache.dubbo.common.constants.CommonConstants.PROVIDER; + +/** + * A {@link Filter} that creates an {@link Observation} around the incoming message. + */ +@Activate(group = PROVIDER, order = -1) +public class ObservationReceiverFilter implements Filter, BaseFilter.Listener, ScopeModelAware { + + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private DubboServerObservationConvention serverObservationConvention = null; + + @Override + public void setApplicationModel(ApplicationModel applicationModel) { Review Comment: The same ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/AbstractDefaultDubboObservationConvention.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.common.KeyValues; +import io.micrometer.common.docs.KeyName; +import io.micrometer.common.lang.Nullable; +import org.apache.dubbo.common.utils.StringUtils; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.RpcContextAttachment; + +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.NET_PEER_NAME; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.NET_PEER_PORT; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_METHOD; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_SERVICE; +import static org.apache.dubbo.metrics.filter.observation.DubboObservation.LowCardinalityKeyNames.RPC_SYSTEM; + +class AbstractDefaultDubboObservationConvention { + KeyValues getLowCardinalityKeyValues(Invocation invocation, RpcContextAttachment rpcContextAttachment) { + KeyValues keyValues = KeyValues.of(RPC_SYSTEM.withValue("apache_dubbo")); + String serviceName = StringUtils.hasText(invocation.getServiceName()) ? invocation.getServiceName() : readServiceName(invocation.getTargetServiceUniqueName()); + keyValues = appendNonNull(keyValues, RPC_SERVICE, serviceName); + keyValues = appendNonNull(keyValues, RPC_METHOD, invocation.getMethodName()); + keyValues = appendNonNull(keyValues, NET_PEER_NAME, rpcContextAttachment.getRemoteHostName()); + if (rpcContextAttachment.getRemotePort() == 0) { + return keyValues; + } + int port = rpcContextAttachment.getRemotePort() != 0 ? rpcContextAttachment.getRemotePort() : rpcContextAttachment.getLocalPort(); + return appendNonNull(keyValues, NET_PEER_PORT, String.valueOf(port)); + } + + private KeyValues appendNonNull(KeyValues keyValues, KeyName keyName, @Nullable String value) { + if (value != null) { + return keyValues.and(keyName.withValue(value)); + } + return keyValues; + } + + String getContextualName(Invocation invocation, RpcContextAttachment rpcContextAttachment) { + String serviceName = StringUtils.hasText(invocation.getServiceName()) ? invocation.getServiceName() : readServiceName(invocation.getTargetServiceUniqueName()); + String method = StringUtils.hasText(rpcContextAttachment.getMethodName()) ? rpcContextAttachment.getMethodName() : invocation.getMethodName(); + return serviceName + "/" + method; + } Review Comment: `Invocation` would be a higher priority. Can ignore `rpcContextAttachment` here. ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/ObservationSenderFilter.java: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import org.apache.dubbo.common.extension.Activate; +import org.apache.dubbo.rpc.BaseFilter; +import org.apache.dubbo.rpc.Filter; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.Result; +import org.apache.dubbo.rpc.RpcContext; +import org.apache.dubbo.rpc.RpcContextAttachment; +import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.cluster.filter.ClusterFilter; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ScopeModelAware; + +import static org.apache.dubbo.common.constants.CommonConstants.CONSUMER; + +/** + * A {@link Filter} that creates an {@link Observation} around the outgoing message. + */ +@Activate(group = CONSUMER, order = -1) +public class ObservationSenderFilter implements ClusterFilter, BaseFilter.Listener, ScopeModelAware { + + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private DubboClientObservationConvention clientObservationConvention = null; + + @Override + public void setApplicationModel(ApplicationModel applicationModel) { + observationRegistry = applicationModel.getBeanFactory().getBean(ObservationRegistry.class); + clientObservationConvention = applicationModel.getBeanFactory().getBean(DubboClientObservationConvention.class); + } + + @Override + public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException { + if (observationRegistry == null) { + return invoker.invoke(invocation); + } + RpcContextAttachment context = RpcContext.getClientAttachment(); + DubboClientContext senderContext = new DubboClientContext(context, invoker, invocation); + Observation observation = DubboObservation.CLIENT.observation(this.clientObservationConvention, DefaultDubboClientObservationConvention.INSTANCE, () -> senderContext, observationRegistry); + return observation.observe(() -> invoker.invoke(invocation)); + } + + @Override + public void onResponse(Result appResponse, Invoker<?> invoker, Invocation invocation) { + + } + + @Override + public void onError(Throwable t, Invoker<?> invoker, Invocation invocation) { Review Comment: Should we track if return errors? ########## dubbo-metrics/dubbo-metrics-api/src/main/java/org/apache/dubbo/metrics/filter/observation/ObservationSenderFilter.java: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.metrics.filter.observation; + +import io.micrometer.observation.Observation; +import io.micrometer.observation.ObservationRegistry; +import org.apache.dubbo.common.extension.Activate; +import org.apache.dubbo.rpc.BaseFilter; +import org.apache.dubbo.rpc.Filter; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.Result; +import org.apache.dubbo.rpc.RpcContext; +import org.apache.dubbo.rpc.RpcContextAttachment; +import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.cluster.filter.ClusterFilter; +import org.apache.dubbo.rpc.model.ApplicationModel; +import org.apache.dubbo.rpc.model.ScopeModelAware; + +import static org.apache.dubbo.common.constants.CommonConstants.CONSUMER; + +/** + * A {@link Filter} that creates an {@link Observation} around the outgoing message. + */ +@Activate(group = CONSUMER, order = -1) +public class ObservationSenderFilter implements ClusterFilter, BaseFilter.Listener, ScopeModelAware { + + private ObservationRegistry observationRegistry = ObservationRegistry.NOOP; + + private DubboClientObservationConvention clientObservationConvention = null; + + @Override + public void setApplicationModel(ApplicationModel applicationModel) { + observationRegistry = applicationModel.getBeanFactory().getBean(ObservationRegistry.class); + clientObservationConvention = applicationModel.getBeanFactory().getBean(DubboClientObservationConvention.class); + } + + @Override + public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcException { + if (observationRegistry == null) { + return invoker.invoke(invocation); + } + RpcContextAttachment context = RpcContext.getClientAttachment(); + DubboClientContext senderContext = new DubboClientContext(context, invoker, invocation); + Observation observation = DubboObservation.CLIENT.observation(this.clientObservationConvention, DefaultDubboClientObservationConvention.INSTANCE, () -> senderContext, observationRegistry); + return observation.observe(() -> invoker.invoke(invocation)); Review Comment: In async mode, `invoker.invoke` would return quickly in a short time and return a furure. If you want to get the actual return time, track in `onResponse` method. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org For additional commands, e-mail: notifications-h...@dubbo.apache.org
