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

Update open telemetry dependencies and switch to using the built in JDK http client for reporting traces #867

Merged
merged 20 commits into from
Mar 6, 2024

Conversation

SamBarker
Copy link
Contributor

@SamBarker SamBarker commented Feb 1, 2024

This PR removes the dependency on OkHttp and thus Kotlin in a fully backwards compatible fashion. It does this by replacing the opentelemetry-exporter-sender-okhttp (which is the OpenTelemetry default) with two separate senders. One for each transport.

  • opentelemetry-exporter-sender-jdk enables sending traces using http/protobuf which is the lowest common denominator option, as its the only method required by the OpenTelemetry specifications.
  • opentelemetry-exporter-sender-grpc-managed-channel enables support for sending traces using grpc and thus feature parity with the OkHttp sender.

In doing so it has brought in a newer version of the SemanticConventions library which has deprecated several of the used conventions. I have added the new fields where appropriate and suppressed the deprecation warnings as we need to propagate the changes to users.

@SamBarker SamBarker force-pushed the jdk-otlp-sender branch 2 times, most recently from 5eb7f3d to 87467ec Compare February 1, 2024 04:23
pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@SamBarker
Copy link
Contributor Author

With fresh eyes I realised there was a way to restore GRPC support (however that does require OpenTelemetry 1.34.0+). By adding dependencies on opentelemetry-exporter-sender-grpc-managed-channel and grpc-netty-shaded we can maintain GRPC support and remove the Kotlin dependency.

The build is now failing due to deprecations in the SemanticAttributes some of them are simple renames however others involve changing the value e.g. SemanticAttributes.HTTP_URL becomes SemanticAttributes.URL_FULL which means its not a backwards compatible change.

Obviously I'm happy to push up the simple renames however I'm looking for input from the maintainers for what to do about the non backwards compatible changes. The build fails due to using the deprecated properties but changing them breaks compatibility.

@ppatierno
Copy link
Member

With fresh eyes I realised there was a way to restore GRPC support (however that does require OpenTelemetry 1.34.0+). By adding dependencies on opentelemetry-exporter-sender-grpc-managed-channel and grpc-netty-shaded we can maintain GRPC support and remove the Kotlin dependency.

Is it a documented way or an hack? Also I think that the deprecation issues makes things more complicated.

@SamBarker
Copy link
Contributor Author

I found it through the pull requests but they are listed in the docs.

The realisation I had was both could be on the classpath and autoconfigure will work out which one it actually needs based on the configuration.

@SamBarker
Copy link
Contributor Author

Oh and as I forgot to mention it earlier the need for 1.34.0 rather than 1.28.0 where the sender was introduced was this bug fix

@SamBarker SamBarker force-pushed the jdk-otlp-sender branch 4 times, most recently from bae934d to 2120a7b Compare February 7, 2024 01:26
@scholzj scholzj added this to the 0.28.0 milestone Feb 8, 2024
pom.xml Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

@SamBarker I had another pass but I see some comments were not addressed.

CHANGELOG.md Outdated Show resolved Hide resolved
@ppatierno
Copy link
Member

@SamBarker I left just a nit and there are conflicts to fix before approving the PR. Thanks!

@SamBarker SamBarker requested a review from ppatierno March 5, 2024 01:32
@SamBarker
Copy link
Contributor Author

@ppatierno @scholzj I believe everything has been addressed and the PR checks are happy modulo a PR approval 😉

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR!

@ppatierno
Copy link
Member

I tested (again) this PR and it seems to work fine, even switching between using grpc (default) to http/protobuf.
@SamBarker could you fix the conflicts please? We want to validate that with the same versions (as you set on strimzi/strimzi-kafka-operator#9758), OpenTelemetry works in the operator as well (KafkaConnect and KafkaMirrorMaker). After this check, I think we can move forward and merge both.

SamBarker and others added 19 commits March 6, 2024 09:30
Switch to using otel provided boms (Bill Of Materials) where possible as that is their supported model for version alignment see: https://arc.net/l/quote/uuvuaxoz

Signed-off-by: Sam Barker <[email protected]>
With both available we are back to feature parity with the OkHttp sender

Signed-off-by: Sam Barker <[email protected]>
Suppresses deprecation warnings as we need to mark them as deprecated to users before deleting them.

Signed-off-by: Sam Barker <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Paolo Patierno <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
… opentelemetry-exporter-otlp.

Signed-off-by: Sam Barker <[email protected]>
Co-authored-by: Paolo Patierno <[email protected]>
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Copy link
Contributor Author

We want to validate that with the same versions (as you set on strimzi/strimzi-kafka-operator#9758), OpenTelemetry works in the operator as well (KafkaConnect and KafkaMirrorMaker). After this check, I think we can move forward and merge both.

I'm not clear what you want me to do, if anything, for that. The versions are the same across both this and the operator PR and the operator regression suite passed. Though that ran with the version of the bridge with out these changes.

@ppatierno
Copy link
Member

I'm not clear what you want me to do, if anything, for that. The versions are the same across both this and the operator PR and the operator regression suite passed. Though that ran with the version of the bridge with out these changes.

Maybe it was a misleading comment but I didn't want nothing to do from you :-D
I meant to wait for the PR on the operator to work fine before merging this one, in order to validate that same OTel versions were working fine on both (operator and bridge).

@ppatierno ppatierno merged commit 825537b into strimzi:main Mar 6, 2024
13 checks passed
@SamBarker SamBarker deleted the jdk-otlp-sender branch March 11, 2024 02:02
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