Skip to content

Use parent Call.Factory of OkHttpClient to be able to use OpenTelemetry - #1252

Merged
Vincent Biret (baywet) merged 5 commits into
microsoftgraph:devfrom
cbos:prepare_for_opentelemetry
Nov 8, 2023
Merged

Use parent Call.Factory of OkHttpClient to be able to use OpenTelemetry#1252
Vincent Biret (baywet) merged 5 commits into
microsoftgraph:devfrom
cbos:prepare_for_opentelemetry

Conversation

@cbos

@cbos Cees Bos (cbos) commented Oct 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Cees Bos (cbos) requested a review from a team as a code owner October 25, 2023 09:14
@cbos Cees Bos (cbos) mentioned this pull request Oct 25, 2023
Closed
@cbos

Cees Bos (cbos) commented Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

baywet
Vincent Biret (baywet) requested changes Oct 25, 2023
View reviewed changes

@baywet Vincent Biret (baywet) left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/main/java/com/microsoft/graph/core/BaseClient.java Outdated
baywet
Vincent Biret (baywet) requested changes Oct 25, 2023
View reviewed changes

@baywet Vincent Biret (baywet) left a comment

Copy link
Copy Markdown
Member

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

Cees Bos (cbos) commented Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

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

Cees Bos (cbos) commented Oct 25, 2023

Copy link
Copy Markdown
Contributor Author

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
Vincent Biret (baywet) previously approved these changes Oct 25, 2023
View reviewed changes

@baywet Vincent Biret (baywet) left a comment

Copy link
Copy Markdown
Member

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!
Ramses Sanchez-Hernandez (@ramsessanchez) for version bump and release.

@cbos

Cees Bos (cbos) commented Oct 26, 2023

Copy link
Copy Markdown
Contributor Author

Vincent Biret (@baywet) Ramses Sanchez-Hernandez (@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

Vincent Biret (baywet) commented Oct 26, 2023

Copy link
Copy Markdown
Member

Cees Bos (@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 Ramses Sanchez-Hernandez (@ramsessanchez) has has time to review it. You can disregard it.

@ramsessanchez

Ramses Sanchez-Hernandez (ramsessanchez) commented Nov 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Looks good! Vincent Biret (@baywet) , Bump to version 2.0.21?

@baywet

Vincent Biret (baywet) commented Nov 2, 2023

Copy link
Copy Markdown
Member

Ramses Sanchez-Hernandez (@ramsessanchez) yes the next patch is fine

@baywet

Vincent Biret (baywet) commented Nov 2, 2023

Copy link
Copy Markdown
Member

Cees Bos (@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 Cees Bos (cbos) mentioned this pull request Nov 8, 2023
Merged
@cbos

Cees Bos (cbos) commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Cees Bos (@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
Vincent Biret (baywet) previously approved these changes Nov 8, 2023
View reviewed changes

@baywet Vincent Biret (baywet) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cees Bos (@cbos) sorry for the delay, Ramses Sanchez-Hernandez (@ramsessanchez) has been busy with the last few items on the next version of the SDK, I'll try to expedite this

@baywet
Vincent Biret (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

Reviewers

@baywet Vincent Biret (baywet) baywet approved these changes
@ramsessanchez Ramses Sanchez-Hernandez (ramsessanchez) ramsessanchez left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cbos @baywet @ramsessanchez