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 native tests #10685

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Fix native tests #10685

merged 5 commits into from
Feb 29, 2024

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 27, 2024

Background: spring starter shouldn't depend on spring-boot-resources, which uses snake yaml, which doesn't work with graal native

@zeitlinger zeitlinger requested a review from a team February 27, 2024 07:31
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Feb 27, 2024
@zeitlinger zeitlinger self-assigned this Feb 27, 2024
@trask
Copy link
Member

trask commented Feb 27, 2024

Looks like native tests still failing?

@zeitlinger
Copy link
Member Author

Looks like native tests still failing?

But not failing in #10636 - flaky?

@jeanbisutti can you take a look?

@jeanbisutti
Copy link
Member

Looks like native tests still failing?

But not failing in #10636 - flaky?

@jeanbisutti can you take a look?

There are two problems leading to the failure of the OpenTelemetry starter tests in the native mode. Please see #10634 (comment).

@trask
Copy link
Member

trask commented Feb 28, 2024

spring starter shouldn't depend on spring-boot-resources, which uses snake yaml, which doesn't work with graal native

isn't the snake yaml problem better solved by splitting out the javaagent resource detector (as done in #10636)?

EDIT thanks @laurit for the explanation #10636 (comment)

@laurit
Copy link
Contributor

laurit commented Feb 28, 2024

spring starter shouldn't depend on spring-boot-resources, which uses snake yaml, which doesn't work with graal native

isn't the snake yaml problem better solved by splitting out the javaagent resource detector (as done in #10636)?

here we remove the dependency on the spring-boot-resources that depends on snakeyaml

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

should we rename spring-boot-resources/library to spring-boot-resources/javaagent? since only intended to be used by java agent?

@laurit
Copy link
Contributor

laurit commented Feb 28, 2024

should we rename spring-boot-resources/library to spring-boot-resources/javaagent? since only intended to be used by java agent?

we use it inside the agent but there is nothing there that prevents it from being used as a library similarly to https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/resources/library

@trask
Copy link
Member

trask commented Feb 28, 2024

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/resources/library seem useful outside of the java agent, but wondering why a non-java agent user would want to use spring-boot-resources/library compared to using BuildProperties-based service name / service version detectors?

@laurit
Copy link
Contributor

laurit commented Feb 28, 2024

https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/resources/library seem useful outside of the java agent, but wondering why a non-java agent user would want to use spring-boot-resources/library compared to using BuildProperties-based service name / service version detectors?

We currently are not publishing these in a separate jar, the only way to get them is to use the autoconfigure module. I do agree that spring-boot-resources has limited usage outside the agent.

@trask
Copy link
Member

trask commented Feb 29, 2024

We currently are not publishing these in a separate jar, the only way to get them is to use the autoconfigure module. I do agree that spring-boot-resources has limited usage outside the agent.

@zeitlinger if you and @jeanbisutti agree, I'd suggest that we rename spring-boot-resources/library to spring-boot-resources/javaagent, and if someone wants to use the BuildProperties-based service name / service version detectors outside of the autoconfigure module, we can add a new spring-boot-resources/library module in the future to contain those

@trask trask merged commit c73bf8e into open-telemetry:main Feb 29, 2024
49 checks passed
@zeitlinger zeitlinger deleted the fix-graal-native branch February 29, 2024 07:42
@zeitlinger
Copy link
Member Author

rename spring-boot-resources/library to spring-boot-resources/javaagent

created #10715

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spring boot starter test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants