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.

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

Add Azure SDK instrumentation #5467

Merged
merged 4 commits into from
Mar 4, 2022
Merged

Add Azure SDK instrumentation #5467

merged 4 commits into from
Mar 4, 2022

Conversation

trask
Copy link
Member

@trask trask commented Feb 28, 2022

No description provided.

Comment on lines +54 to +68
// we cannot use com.azure.core.http.policy.AfterRetryPolicyProvider
// or com.azure.core.util.tracing.Tracer here because we inject classes that implement these
// interfaces, causing the first one of these interfaces to be transformed to cause itself to
// be loaded (again), which leads to duplicate class definition error after the interface is
// transformed and the triggering class loader tries to load it.
//
// this is a list of all classes that call one of these:
// * ServiceLoader.load(AfterRetryPolicyProvider.class)
// * ServiceLoader.load(Tracer.class)
return named("com.azure.core.http.policy.HttpPolicyProviders")
.or(named("com.azure.core.util.tracing.TracerProxy"))
.or(named("com.azure.cosmos.CosmosAsyncClient"))
.or(named("com.azure.messaging.eventhubs.EventHubClientBuilder"))
.or(named("com.azure.messaging.eventhubs.EventProcessorClientBuilder"))
.or(named("com.azure.messaging.servicebus.ServiceBusClientBuilder"));
Copy link
Member Author

@trask trask Feb 28, 2022

Choose a reason for hiding this comment

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

I thought this could be simplified similar to #5216 (thanks to #5185), but an issue (#5466) was just opened about #5216 causing a regression

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can simplify it when we switch to instrumentation-api here

@trask trask marked this pull request as ready for review February 28, 2022 19:27
@trask trask requested review from a team and lmolkova February 28, 2022 19:27
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

As per offline discussion, we should keep suppression in 1.19 version. Otherwise looks great!

Comment on lines +54 to +68
// we cannot use com.azure.core.http.policy.AfterRetryPolicyProvider
// or com.azure.core.util.tracing.Tracer here because we inject classes that implement these
// interfaces, causing the first one of these interfaces to be transformed to cause itself to
// be loaded (again), which leads to duplicate class definition error after the interface is
// transformed and the triggering class loader tries to load it.
//
// this is a list of all classes that call one of these:
// * ServiceLoader.load(AfterRetryPolicyProvider.class)
// * ServiceLoader.load(Tracer.class)
return named("com.azure.core.http.policy.HttpPolicyProviders")
.or(named("com.azure.core.util.tracing.TracerProxy"))
.or(named("com.azure.cosmos.CosmosAsyncClient"))
.or(named("com.azure.messaging.eventhubs.EventHubClientBuilder"))
.or(named("com.azure.messaging.eventhubs.EventProcessorClientBuilder"))
.or(named("com.azure.messaging.servicebus.ServiceBusClientBuilder"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can simplify it when we switch to instrumentation-api here

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Wouldn't block it but want to point out that there seems to be quite a small span between 1.14 (8/2018) and 1.19 (1/2019) that having multiple instrumentation seems a bit overkill.

@trask
Copy link
Member Author

trask commented Mar 3, 2022

Wouldn't block it but want to point out that there seems to be quite a small span between 1.14 (8/2018) and 1.19 (1/2019) that having multiple instrumentation seems a bit overkill.

those dates don't look right to me: https://search.maven.org/artifact/com.azure/azure-core

@anuraaga
Copy link
Contributor

anuraaga commented Mar 3, 2022

Hmm, I got it from mvnrepository.com which surprisingly is more accurate it seems

https://mvnrepository.com/artifact/com.microsoft.azure/azure/1.14.0
https://repo1.maven.org/maven2/com/microsoft/azure/azure/

@trask
Copy link
Member Author

trask commented Mar 3, 2022

oh, you're looking at the wrong groupId/artifactId

@anuraaga
Copy link
Contributor

anuraaga commented Mar 3, 2022

Oops that explains it. So I guess 1.14 being less than a year old, so pretty new, is motivation to instrument it that seems reasonable.

@trask trask merged commit ab9169c into open-telemetry:main Mar 4, 2022
@trask trask deleted the azure-sdk branch March 4, 2022 21:08
@trask
Copy link
Member Author

trask commented Mar 4, 2022

Ooops, I forgot was going to convert tests to Java, will send separate PR.

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Add Azure SDK instrumentation

* Add to supported libraries table

* Keep suppression for 1.19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants