Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix wrong dubbo trace caused by using rpcContext.isProviderSide() #12930

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
37808d7
Fix wrong dubbo trace caused by using rpcContext.isProviderSide()
steverao Dec 21, 2024
92583fc
Address comment problems
steverao Dec 23, 2024
f5fafc0
Keep old api and mark it as deprecated
steverao Dec 25, 2024
93f2ffb
Optimize codes
steverao Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .../opentelemetry/javaagent/instrumentation/apachedubbo/v2_7/DubboInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
public void injectClasses(ClassInjector injector) {
injector
.proxyBuilder(
"io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryFilter")
"io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryClientFilter")
.inject(InjectionMode.CLASS_ONLY);
injector
.proxyBuilder(
"io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryServerFilter")
.inject(InjectionMode.CLASS_ONLY);
}

Expand Down
8 changes: 4 additions & 4 deletions ...apachedubbo/v2_7/OpenTelemetryFilter.java → ...dubbo/v2_7/OpenTelemetryClientFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,20 @@
import org.apache.dubbo.rpc.Invoker;
import org.apache.dubbo.rpc.Result;

@Activate(group = {"consumer", "provider"})
public class OpenTelemetryFilter implements Filter {
@Activate(group = {"consumer"})
public class OpenTelemetryClientFilter implements Filter {
steverao marked this conversation as resolved.
Show resolved Hide resolved

private final Filter delegate;

public OpenTelemetryFilter() {
public OpenTelemetryClientFilter() {
delegate =
DubboTelemetry.builder(GlobalOpenTelemetry.get())
.addAttributesExtractor(
PeerServiceAttributesExtractor.create(
new DubboClientNetworkAttributesGetter(),
AgentCommonConfig.get().getPeerServiceResolver()))
.build()
.newFilter();
.newClientFilter();
}

@Override
Expand Down
39 changes: 39 additions & 0 deletions ...o/opentelemetry/javaagent/instrumentation/apachedubbo/v2_7/OpenTelemetryServerFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.instrumentation.apachedubbo.v2_7.DubboTelemetry;
import io.opentelemetry.instrumentation.apachedubbo.v2_7.internal.DubboClientNetworkAttributesGetter;
import io.opentelemetry.instrumentation.api.incubator.semconv.net.PeerServiceAttributesExtractor;
import io.opentelemetry.javaagent.bootstrap.internal.AgentCommonConfig;
import org.apache.dubbo.common.extension.Activate;
import org.apache.dubbo.rpc.Filter;
import org.apache.dubbo.rpc.Invocation;
import org.apache.dubbo.rpc.Invoker;
import org.apache.dubbo.rpc.Result;

@Activate(group = {"provider"})
public class OpenTelemetryServerFilter implements Filter {
steverao marked this conversation as resolved.
Show resolved Hide resolved

private final Filter delegate;

public OpenTelemetryServerFilter() {
Copy link
Contributor

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?

Copy link
Contributor Author

@steverao steverao Dec 24, 2024 •
edited
Loading

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. Or leave it as it's? Do you have any suggestion here?

delegate =
DubboTelemetry.builder(GlobalOpenTelemetry.get())
.addAttributesExtractor(
PeerServiceAttributesExtractor.create(
new DubboClientNetworkAttributesGetter(),
AgentCommonConfig.get().getPeerServiceResolver()))
.build()
.newServerFilter();
}

@Override
public Result invoke(Invoker<?> invoker, Invocation invocation) {
return delegate.invoke(invoker, invocation);
}
}
3 changes: 2 additions & 1 deletion ...bo-2.7/javaagent/src/main/resources/apache-dubbo-2.7/META-INF/org.apache.dubbo.rpc.Filter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryFilter
io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryClientFilter
io.opentelemetry.javaagent.instrumentation.apachedubbo.v2_7.OpenTelemetryServerFilter
12 changes: 9 additions & 3 deletions ...igure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboTelemetry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public static DubboTelemetryBuilder builder(OpenTelemetry openTelemetry) {
}

private final Instrumenter<DubboRequest, Result> serverInstrumenter;

steverao marked this conversation as resolved.
Show resolved Hide resolved
private final Instrumenter<DubboRequest, Result> clientInstrumenter;

DubboTelemetry(
Expand All @@ -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() {
Copy link
Contributor

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.

Copy link
Contributor Author

@steverao steverao Dec 24, 2024 •
edited
Loading

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.

Copy link
Member

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

Copy link
Contributor Author

@steverao steverao Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your reminder and explanation, I have adjusted it and please have a look after holiday. Merry Christmas!

return new TracingFilter(serverInstrumenter, clientInstrumenter);
/** Returns a new Dubbo client {@link Filter} that traces Dubbo RPC invocations. */
public Filter newClientFilter() {
return new TracingFilter(clientInstrumenter, true);
}

/** Returns a new Dubbo server {@link Filter} that traces Dubbo RPC invocations. */
public Filter newServerFilter() {
return new TracingFilter(serverInstrumenter, false);
}
}
8 changes: 4 additions & 4 deletions ...apachedubbo/v2_7/OpenTelemetryFilter.java → ...dubbo/v2_7/OpenTelemetryClientFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@
import org.apache.dubbo.rpc.Invoker;
import org.apache.dubbo.rpc.Result;

@Activate(group = {"consumer", "provider"})
public final class OpenTelemetryFilter implements Filter {
@Activate(group = {"consumer"})
public final class OpenTelemetryClientFilter implements Filter {

private final Filter delegate;

public OpenTelemetryFilter() {
delegate = DubboTelemetry.create(GlobalOpenTelemetry.get()).newFilter();
public OpenTelemetryClientFilter() {
delegate = DubboTelemetry.create(GlobalOpenTelemetry.get()).newClientFilter();
}

@Override
Expand Down
28 changes: 28 additions & 0 deletions ...ain/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/OpenTelemetryServerFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.apachedubbo.v2_7;

import io.opentelemetry.api.GlobalOpenTelemetry;
import org.apache.dubbo.common.extension.Activate;
import org.apache.dubbo.rpc.Filter;
import org.apache.dubbo.rpc.Invocation;
import org.apache.dubbo.rpc.Invoker;
import org.apache.dubbo.rpc.Result;

@Activate(group = {"provider"})
public final class OpenTelemetryServerFilter implements Filter {

private final Filter delegate;

public OpenTelemetryServerFilter() {
delegate = DubboTelemetry.create(GlobalOpenTelemetry.get()).newServerFilter();
}

@Override
public Result invoke(Invoker<?> invoker, Invocation invocation) {
return delegate.invoke(invoker, invocation);
}
}
17 changes: 6 additions & 11 deletions ...figure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/TracingFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@

final class TracingFilter implements Filter {

private final Instrumenter<DubboRequest, Result> serverInstrumenter;
private final Instrumenter<DubboRequest, Result> clientInstrumenter;
private final Instrumenter<DubboRequest, Result> instrumenter;
private final boolean isClient;

TracingFilter(
Instrumenter<DubboRequest, Result> serverInstrumenter,
Instrumenter<DubboRequest, Result> clientInstrumenter) {
this.serverInstrumenter = serverInstrumenter;
this.clientInstrumenter = clientInstrumenter;
TracingFilter(Instrumenter<DubboRequest, Result> instrumenter, boolean isClient) {
this.instrumenter = instrumenter;
this.isClient = isClient;
}

@Override
Expand All @@ -40,9 +38,6 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) {
return invoker.invoke(invocation);
}

boolean isServer = rpcContext.isProviderSide();
Instrumenter<DubboRequest, Result> instrumenter =
isServer ? serverInstrumenter : clientInstrumenter;
Context parentContext = Context.current();
DubboRequest request = DubboRequest.create((RpcInvocation) invocation, rpcContext);

Expand All @@ -55,7 +50,7 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) {
boolean isSynchronous = true;
try (Scope ignored = context.makeCurrent()) {
result = invoker.invoke(invocation);
if (!isServer) {
if (isClient) {
CompletableFuture<Object> future = rpcContext.getCompletableFuture();
if (future != null) {
isSynchronous = false;
Expand Down
3 changes: 2 additions & 1 deletion ....7/library-autoconfigure/src/main/resources/META-INF/services/org.apache.dubbo.rpc.Filter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
io.opentelemetry.instrumentation.apachedubbo.v2_7.OpenTelemetryFilter
io.opentelemetry.instrumentation.apachedubbo.v2_7.OpenTelemetryClientFilter
io.opentelemetry.instrumentation.apachedubbo.v2_7.OpenTelemetryServerFilter
Loading