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

Fix spring boot starter failing without logback #10802

Merged
merged 3 commits into from
Mar 12, 2024

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 11, 2024

Resolves #10784
Resolves #10612 (resolves the stack trace in #10612 (comment) otel.sdk.disabled part seems to already have been implemented in some other pr)

@laurit laurit requested a review from a team March 11, 2024 13:51
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Mar 11, 2024
@laurit laurit requested a review from jeanbisutti March 11, 2024 13:58
@jeanbisutti jeanbisutti reopened this Mar 11, 2024
Copy link
Member

@zeitlinger zeitlinger left a comment

Choose a reason for hiding this comment

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

I think we should have a test that covers this scenario - maybe in smoke-tests-otel-starter

@laurit
Copy link
Contributor Author

laurit commented Mar 11, 2024

@zeitlinger I'm not sure how to test this. Perhaps you or @jeanbisutti can help with it? I definitely won't be able to make it for this release cycle.

@zeitlinger
Copy link
Member

@zeitlinger I'm not sure how to test this. Perhaps you or @jeanbisutti can help with it? I definitely won't be able to make it for this release cycle.

I'll prepare a PR targeting your branch

@jeanbisutti
Copy link
Member

For several reasons (one of them is to not test most of the things from one test project) it would make sense to test this issue in the same way as https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender. @zeitlinger Can I do this or would you like to do this?

@laurit
Copy link
Contributor Author

laurit commented Mar 11, 2024

For several reasons (one of them is to not test most of the things from one test project) it would make sense to test this issue in the same way as https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender. @zeitlinger Can I do this or would you like to do this?

I think this would be better than creating a smoke test that would require separate image. The main hurdle in creating this test would be getting rid of the logback from the test class path.

@laurit
Copy link
Contributor Author

laurit commented Mar 11, 2024

@zeitlinger I added a test, hope this is enough.

@trask trask added this to the v2.2.0 milestone Mar 11, 2024
@zeitlinger
Copy link
Member

@zeitlinger I added a test, hope this is enough.

It's better than what I came up with 😄

Only makes me a little bit depressed that gradle is so hard...

@laurit laurit merged commit fa5548b into open-telemetry:main Mar 12, 2024
49 checks passed
@laurit laurit deleted the spring-starter-logback branch March 12, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
4 participants