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

[PF-3006] Manually migrate to 2.6.0 version of open telemetry #196

Closed
wants to merge 2 commits into from

Conversation

nmalfroy
Copy link
Contributor

Note: io.opentelemetry.instrumentation.spring.autoconfigure.EnableOpenTelemetry was removed in this version but presumably should not be needed. May be worth checking if this continues to work (not entirely sure how to test that)

Note: io.opentelemetry.instrumentation.spring.autoconfigure.EnableOpenTelemetry was removed in this version but presumably should not be needed.  May be worth checking if this continues to work (not entirely sure how to test that)
Per the release notes for 2.6.0
Copy link

Copy link
Contributor

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

giving this a 👍 because I know all you're doing is making dependency versions work.

However, I note that https://opentelemetry.io/docs/zero-code/java/spring-boot-starter/getting-started/#dependency-management says:

Be careful not to mix up the different ways of configuring things with Gradle. For example, don’t use implementation(platform("io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.6.0")) with the io.spring.dependency-management plugin

which is exactly what we're doing. They recommend putting the otel boms under a dependencyManagement stanza.

re: testing, I also don't know how to test this manually but assume the tests under bio.terra.common.tracing cover this. In fact, if I add otel.sdk.disabled=true to application-tracing-test.properties, I can see a test fail.

@nmalfroy
Copy link
Contributor Author

giving this a 👍 because I know all you're doing is making dependency versions work.

However, I note that https://opentelemetry.io/docs/zero-code/java/spring-boot-starter/getting-started/#dependency-management says:

Be careful not to mix up the different ways of configuring things with Gradle. For example, don’t use implementation(platform("io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.6.0")) with the io.spring.dependency-management plugin

which is exactly what we're doing. They recommend putting the otel boms under a dependencyManagement stanza.

re: testing, I also don't know how to test this manually but assume the tests under bio.terra.common.tracing cover this. In fact, if I add otel.sdk.disabled=true to application-tracing-test.properties, I can see a test fail.

Yeah, I was thinking there's probably an overhaul due but I was worried about too much with the initial scope of the PR. I worry about how that could affect import behavior for services

@nmalfroy nmalfroy requested a review from cahrens August 22, 2024 16:44
@cahrens
Copy link
Contributor

cahrens commented Aug 22, 2024

@marctalbott is working some with open telemetry in #197. So he might be able to provide some further verification that this change doesn't break things.

@marctalbott
Copy link
Member

@marctalbott is working some with open telemetry in #197. So he might be able to provide some further verification that this change doesn't break things.

I am not sure if there is a better way to test this, but I've been publishing TCL locally and pulling it into RBS which I run locally and check for metrics by curling localhost:9098/metrics. I haven't dug into it at all, but I tried doing that with this change and RBS failed to start with this message:

***************************
APPLICATION FAILED TO START
***************************

Description:

The bean 'logbackOtelAppenderInitializer', defined in class path resource [io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/OpenTelemetryAppenderAutoConfiguration$LogbackAppenderConfig.class], could not be registered. A bean with that name has already been defined in class path resource [io/opentelemetry/instrumentation/spring/autoconfigure/instrumentation/logging/OpenTelemetryAppenderAutoConfiguration$LogbackAppenderConfig.class] and overriding is disabled.

Action:

Consider renaming one of the beans or enabling overriding by setting spring.main.allow-bean-definition-overriding=true

@pshapiro4broad
Copy link
Member

pshapiro4broad commented Sep 6, 2024

@marctalbott I've been looking into this for #198 and have been able to get it working locally. What worked for me in RBS is to remove all the otel dependencies from its build.gradle. I'll get the other PR in shape and then PR into RBS. Not sure what other services might need updates though.

@pshapiro4broad
Copy link
Member

Closing this as as its changes were superseded by #198

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.

5 participants