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

Jump to bottom

Update azure-core-tracing-opentelemetry version and fix sync suppression #10350

Merged
merged 5 commits into from
Feb 1, 2024
Merged

Update azure-core-tracing-opentelemetry version and fix sync suppression #10350

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bc7824
Update azure-core-tracing-opentelemetry version and fix sync suppression
lmolkova Jan 30, 2024
9b66ec0
update azure-core version for consistency?
lmolkova Jan 30, 2024
a20fda5
add instrumentation for azure-core-1.40
laurit Jan 31, 2024
b1bec2d
Stub class that tiggers muzzle failure
laurit Feb 1, 2024
778fc3e
update settings.gradle.kts
laurit Feb 1, 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
2 changes: 1 addition & 1 deletion instrumentation/azure-core/azure-core-1.36/javaagent/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ sourceSets {
dependencies {
compileOnly(project(":instrumentation:azure-core:azure-core-1.36:library-instrumentation-shaded", configuration = "shadow"))

library("com.azure:azure-core:1.36.0")
library("com.azure:azure-core:1.45.1")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not really necessary and just keeps azure-core in sync with azure-core-tracing-opentelemetry. azure-core is not shaded and is compatible with old version.

So 1.36 is still the min valid version for this code, but should I also update the project name since I'm changing the current lib version?

Copy link
Member

Choose a reason for hiding this comment

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

I'd keep 1.36.0 here, tests are already run against latest version of library dependencies

Copy link
Contributor

Choose a reason for hiding this comment

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

This version here is only really used for compiling, the version used for tests is in testAzure task. We are using testAzure instead of test because the shaded azure-core-tracing-opentelemetry dependency ended in test classpath due to which it was hard to verify whether these classes are found from the agent as they should be of from the test classpath.
Looking at the failures I think we need to introduce azure-core-1.40 module, the new azure-core-tracing-opentelemetry isn't compatible with azure-core-1.36. I'll push adding azure-core-1.40 to this PR, hope you don't mind.

Copy link
Contributor Author

@lmolkova lmolkova Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot @laurit !

the new azure-core-tracing-opentelemetry isn't compatible with azure-core-1.36

it is not but only on the manual configuration path where it never affects the agent.
I was wondering if it's possible to suppress muzzle warning and ignore this specific mismatch.

But I'm going to add another module for azure-core 1.40 or 1.41 in a couple of weeks and we can revisit this.

So thanks a lot for the fix and feel free to merge if you're happy with it.

Copy link
Member

Choose a reason for hiding this comment

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

it looks like muzzle scans all methods in all helper classes

when the helper classes are implemented in this repo we can just remove unused methods (like the OpenTelemetryTracingOptions constructor that is causing issues but isn't needed by the agent instrumentation)

maybe we could "remove" unnecessary methods when we shade the helper classes, but not sure how to do that easily, so splitting out a new module makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

I remove the azure-core-1.40 module and instead replaced the OpenTelemetryTracingOptions that caused muzzle failures with a stub. Please review and decide which approach you like better.

Copy link
Member

Choose a reason for hiding this comment

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

that looks nice to me, thx, will give @lmolkova the final word 👍

Copy link
Contributor Author

@lmolkova lmolkova Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ah nice trick @laurit! Looks great! Give me 30 mins to pull, build, and quickly test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried it and everything looks great. Thanks a lot!


// Ensure no cross interference
testInstrumentation(project(":instrumentation:azure-core:azure-core-1.14:javaagent"))
Expand Down
33 changes: 29 additions & 4 deletions ...entelemetry/javaagent/instrumentation/azurecore/v1_36/AzureHttpClientInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
package io.opentelemetry.javaagent.instrumentation.azurecore.v1_36;

import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.instrumentation.azurecore.v1_36.SuppressNestedClientHelper.disallowNestedClientSpanMono;
import static io.opentelemetry.javaagent.instrumentation.azurecore.v1_36.SuppressNestedClientHelper.disallowNestedClientSpanSync;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;

import com.azure.core.http.HttpResponse;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import net.bytebuddy.asm.Advice;
Expand All @@ -33,15 +36,37 @@ public void transform(TypeTransformer transformer) {
.and(isPublic())
.and(named("send"))
.and(returns(named("reactor.core.publisher.Mono"))),
this.getClass().getName() + "$SuppressNestedClientAdvice");
this.getClass().getName() + "$SuppressNestedClientMonoAdvice");
transformer.applyAdviceToMethod(
isMethod()
.and(isPublic())
.and(named("sendSync"))
.and(returns(named("com.azure.core.http.HttpResponse"))),
this.getClass().getName() + "$SuppressNestedClientSyncAdvice");
}

@SuppressWarnings("unused")
public static class SuppressNestedClientMonoAdvice {
@Advice.OnMethodExit(suppress = Throwable.class)
public static void asyncSendExit(@Advice.Return(readOnly = false) Mono<HttpResponse> mono) {
mono = disallowNestedClientSpanMono(mono);
}
}

@SuppressWarnings("unused")
public static class SuppressNestedClientAdvice {
public static class SuppressNestedClientSyncAdvice {

@Advice.OnMethodEnter(suppress = Throwable.class)
public static void syncSendEnter(@Advice.Local("otelScope") Scope scope) {
scope = disallowNestedClientSpanSync();
}

@Advice.OnMethodExit(suppress = Throwable.class)
public static void methodExit(@Advice.Return(readOnly = false) Mono<HttpResponse> mono) {
mono = new SuppressNestedClientMono<>(mono);
public static void syncSendExit(
@Advice.Return HttpResponse response, @Advice.Local("otelScope") Scope scope) {
if (scope != null) {
scope.close();
}
}
}
}
36 changes: 22 additions & 14 deletions ...ecore/v1_36/SuppressNestedClientMono.java → ...ore/v1_36/SuppressNestedClientHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,30 @@
import reactor.core.CoreSubscriber;
import reactor.core.publisher.Mono;

public class SuppressNestedClientMono<T> extends Mono<T> {
public class SuppressNestedClientHelper {

private final Mono<T> delegate;

public SuppressNestedClientMono(Mono<T> delegate) {
this.delegate = delegate;
}

@Override
public void subscribe(CoreSubscriber<? super T> actual) {
public static Scope disallowNestedClientSpanSync() {
Context parentContext = currentContext();
if (doesNotHaveClientSpan(parentContext)) {
try (Scope ignored = disallowNestedClientSpan(parentContext).makeCurrent()) {
delegate.subscribe(actual);
}
} else {
delegate.subscribe(actual);
return disallowNestedClientSpan(parentContext).makeCurrent();
}
return null;
}

public static <T> Mono<T> disallowNestedClientSpanMono(Mono<T> delegate) {
return new Mono<T>() {
@Override
public void subscribe(CoreSubscriber<? super T> coreSubscriber) {
Context parentContext = currentContext();
if (doesNotHaveClientSpan(parentContext)) {
try (Scope ignored = disallowNestedClientSpan(parentContext).makeCurrent()) {
delegate.subscribe(coreSubscriber);
}
} else {
delegate.subscribe(coreSubscriber);
}
}
};
}

private static boolean doesNotHaveClientSpan(Context parentContext) {
Expand All @@ -44,4 +50,6 @@ private static Context disallowNestedClientSpan(Context parentContext) {
return SpanKey.HTTP_CLIENT.storeInContext(
SpanKey.KIND_CLIENT.storeInContext(parentContext, span), span);
}

private SuppressNestedClientHelper() {}
}
2 changes: 1 addition & 1 deletion instrumentation/azure-core/azure-core-1.36/library-instrumentation-shaded/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ plugins {
group = "io.opentelemetry.javaagent.instrumentation"

dependencies {
implementation("com.azure:azure-core-tracing-opentelemetry:1.0.0-beta.32")
implementation("com.azure:azure-core-tracing-opentelemetry:1.0.0-beta.42")
}

tasks {
Expand Down
Loading