-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix wrong dubbo trace caused by using rpcContext.isProviderSide() #12930
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server and client filter are almost identical, did you consider sharing some code between them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Resolves #12928, I split the
OpenTelemetryFilter
intoOpenTelemetryClientFilter
andOpenTelemetryServerFilter
and avoid usingrpcContext.isProviderSide()
.