Fix wrong dubbo trace caused by using rpcContext.isProviderSide() #12930
Conversation
...onfigure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboTelemetry.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/apachedubbo/v2_7/OpenTelemetryClientFilter.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/javaagent/instrumentation/apachedubbo/v2_7/OpenTelemetryServerFilter.java
Outdated
Show resolved
Hide resolved
|
|
||
| private final Filter delegate; | ||
|
|
||
| public OpenTelemetryServerFilter() { |
There was a problem hiding this comment.
server and client filter are almost identical, did you consider sharing some code between them?
There was a problem hiding this comment.
server and client filter are almost identical, did you consider sharing some code between them?
Make sense, compared with other filters in library. It added extra attribute and causes a bit redundant here. If we extract a basic filter that contains them. The structure looks a bit complex. Another solution is to extract a method and put it into other class like DubboTelemetry, but I don't have a good method naming. So I am torn about them. Or leave it as it's? Do you have any suggestion here?
...onfigure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboTelemetry.java
Outdated
Show resolved
Hide resolved
| @@ -35,8 +36,13 @@ public static DubboTelemetryBuilder builder(OpenTelemetry openTelemetry) { | |||
| this.clientInstrumenter = clientInstrumenter; | |||
| } | |||
|
|
|||
| /** Returns a new Dubbo {@link Filter} that traces Dubbo RPC invocations. */ | |||
| public Filter newFilter() { | |||
There was a problem hiding this comment.
Usually we mark the old method as deprecated, keep it around for at least one release, and then remove.
There was a problem hiding this comment.
Usually we mark the old method as deprecated, keep it around for at least one release, and then remove.
I find it often appears in some situations where a change from experimental to stable. This is a bug here, I think we shouldn't allow users to use the method again.
In addition, if it's necessary here, I found TracingFilter might need to keep the original invoke implementation again. I am not sure whether it's a bit strange.
There was a problem hiding this comment.
one of the reasons we deprecate first before removing (when possible) is that we use the @deprecated Javadoc to tell users which method to use instead. you can remove the method in the following minor release
There was a problem hiding this comment.
Thanks for your reminder and explanation, I have adjusted it and please have a look after holiday. Merry Christmas!
Resolves #12928, I split the
OpenTelemetryFilterintoOpenTelemetryClientFilterandOpenTelemetryServerFilterand avoid usingrpcContext.isProviderSide().