Skip to content

Conversation

@cbos
Copy link
Contributor

@cbos cbos commented Oct 25, 2023

For application insights it is good to be able to get insights in all incoming and outgoing requests.
OpenTelemetry is the standard for that.

I tried to use https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/okhttp/okhttp-3.0/library/README.md

  private Call.Factory createTracedClient(OpenTelemetry openTelemetry, @Nonnull final IAuthenticationProvider auth) {
        return OkHttpTelemetry.builder(openTelemetry).build().newCallFactory(createClient(auth));
    }

  private OkHttpClient createClient(@Nonnull final IAuthenticationProvider auth) {
        return HttpClients.createDefault(auth);
    }

// then create the GraphServiceClient
IAuthenticationProvider authenticationProvider = requestUrl -> cf;
GraphServiceClient
                .builder(Call.Factory.class, Request.class)
                .httpClient(createTracedClient(openTelemetry, authenticationProvider))
                .authenticationProvider(authenticationProvider)
                .buildClient();

This gives an exception at runtime

Caused by: java.lang.ClassCastException: class io.opentelemetry.instrumentation.okhttp.v3_0.TracingCallFactory cannot be cast to class okhttp3.OkHttpClient (io.opentelemetry.instrumentation.okhttp.v3_0.TracingCallFactory and okhttp3.OkHttpClient are in unnamed module of loader io.quarkus.bootstrap.classloading.QuarkusClassLoader @4df50bcc)
        at com.microsoft.graph.core.BaseClient$Builder.getHttpProvider(BaseClient.java:188)
        at com.microsoft.graph.core.BaseClient$Builder.buildClient(BaseClient.java:275)
        at com.microsoft.graph.requests.GraphServiceClient$Builder.buildClient(GraphServiceClient.java:153)

But there is no actual use of casting it to OkHttpClient, only the methods of Call.Factory are used.

As that is the minimal base required, I changed the code as well that in the generic it requires as least Call.Factory as base.

@cbos
Copy link
Contributor Author

cbos commented Oct 25, 2023

@microsoft-github-policy-service agree

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Can you also add an entry to the changelog?
Lastly you want to check https://github.com/microsoft/kiota and https://github.com/microsoft/kiota-java which is the new stack the next version of the Java SDK is going to rely on.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, would you mind adding an entry to the changelog as well please?

@cbos
Copy link
Contributor Author

cbos commented Oct 25, 2023

Thanks for the contribution. Can you also add an entry to the changelog? Lastly you want to check https://github.com/microsoft/kiota and https://github.com/microsoft/kiota-java which is the new stack the next version of the Java SDK is going to rely on.

I have had a look at https://github.com/microsoft/kiota-java.
At first glace that is more tight to OkHttpClient, but it looks like it is also prepared for observability with OpenTelemetry:
See for example https://github.com/microsoft/kiota-java/blob/main/components/http/okHttp/src/main/java/com/microsoft/kiota/http/middleware/ObservabilityHelper.java
A number of classes do import some classes from OpenTelemetry like io.opentelemetry.api.trace.Span.

@cbos
Copy link
Contributor Author

cbos commented Oct 25, 2023

Thanks for making the changes, would you mind adding an entry to the changelog as well please?

Yes, did that as well (but got distracted at home in the mean time), just pushed that change.
I included some instructions on how to use it with OpenTelemetry.

I just the changed code already locally and that works fine, I got traces with OpenTelemetry for the calls send with msgraph sdk library.

baywet
baywet previously approved these changes Oct 25, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes! And for the contribution!
@ramsessanchez for version bump and release.

@cbos
Copy link
Contributor Author

cbos commented Oct 26, 2023

@baywet @ramsessanchez

It looks like there are some issues with Sonar code scanning integration for building this PR.
That gave some issues with the last commit on dev branch as well.

@baywet
Copy link
Member

baywet commented Oct 26, 2023

@cbos this is because the PR is coming from a fork and the policy doesn't allow access to secrets in that context for security reasons. we'll have to override the check to merge this when @ramsessanchez has has time to review it. You can disregard it.

@ramsessanchez
Copy link
Contributor

ramsessanchez commented Nov 1, 2023

Looks good! @baywet , Bump to version 2.0.21?

ramsessanchez
ramsessanchez previously approved these changes Nov 1, 2023
@baywet
Copy link
Member

baywet commented Nov 2, 2023

@ramsessanchez yes the next patch is fine

@baywet
Copy link
Member

baywet commented Nov 2, 2023

@cbos would you be willing to submit a similar pull request to kiota-java (the infrastructure for the next version of the SDK we've been working on)

@cbos
Copy link
Contributor Author

cbos commented Nov 8, 2023

@cbos would you be willing to submit a similar pull request to kiota-java (the infrastructure for the next version of the SDK we've been working on)

@baywat
Done, see microsoft/kiota-java#805
But the need is less for Kiota, as that already uses OpenTelemetry throughout the code.

This PR is not merged yet, any idea when that will happen?

@baywet baywet dismissed stale reviews from ramsessanchez and themself via 69294af November 8, 2023 13:31
@baywet baywet force-pushed the prepare_for_opentelemetry branch from 40a1482 to 69294af Compare November 8, 2023 13:31
baywet
baywet previously approved these changes Nov 8, 2023
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

@cbos sorry for the delay, @ramsessanchez has been busy with the last few items on the next version of the SDK, I'll try to expedite this

@baywet baywet enabled auto-merge November 8, 2023 13:32
@baywet baywet merged commit 43eba0b into microsoftgraph:dev Nov 8, 2023
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