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.

Sign up for GitHub

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

Jump to bottom

Split spring resource detectors #10636

Closed
wants to merge 8 commits into from
Closed

Split spring resource detectors #10636

wants to merge 8 commits into from

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Feb 22, 2024 •
edited
Loading

because one of them should only be used in the javaagent

Fixes #10634

@zeitlinger zeitlinger self-assigned this Feb 22, 2024
@jeanbisutti jeanbisutti mentioned this pull request Feb 22, 2024
Closed
@zeitlinger zeitlinger force-pushed the split-spring-resource-detectors branch 2 times, most recently from aec3370 to 927bc66 Compare February 23, 2024 15:54
@zeitlinger zeitlinger marked this pull request as ready for review February 23, 2024 15:54
@zeitlinger zeitlinger requested a review from a team February 23, 2024 15:54
@jeanbisutti jeanbisutti added the test native This label can be applied to PRs to trigger them to run native tests label Feb 23, 2024
@jeanbisutti jeanbisutti reopened this Feb 23, 2024
@zeitlinger zeitlinger force-pushed the split-spring-resource-detectors branch from 927bc66 to 940de9a Compare February 26, 2024 12:18
trask
trask reviewed Feb 26, 2024
View reviewed changes
instrumentation/spring/spring-boot-resources/library/build.gradle.kts
Comment on lines -12 to -13
implementation("org.snakeyaml:snakeyaml-engine")

Copy link
Member

Choose a reason for hiding this comment

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

👍

instrumentation/spring/spring-boot-resources/testing/build.gradle.kts
testImplementation(project(":instrumentation:spring:spring-boot-resources:library"))
testImplementation(project(":instrumentation:spring:spring-boot-resources:javaagent"))
Copy link
Member

Choose a reason for hiding this comment

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

since this module only contains javaagent tests(?), let's move them to the javaagent module and delete the testing module

Copy link
Member Author

Choose a reason for hiding this comment

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

This breaks the test, because it expects a file to be missing - but it's present in the javaagent test dir

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see, maybe this testing module should be renamed to javaagent-unit-tests? https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/writing-instrumentation.md#writing-java-agent-unit-tests

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@trask
Copy link
Member

trask commented Feb 26, 2024

when you have a chance can you check CI failures?

@zeitlinger
Copy link
Member Author

when you have a chance can you check CI failures?

done - and now another test failure from broken main (Kafka related)

@zeitlinger zeitlinger force-pushed the split-spring-resource-detectors branch from d59c9b9 to e33e9e0 Compare February 27, 2024 10:44
@zeitlinger zeitlinger mentioned this pull request Feb 28, 2024
Merged
@zeitlinger
Copy link
Member Author

Closed in favor of #10685

@zeitlinger zeitlinger closed this Feb 28, 2024
@zeitlinger zeitlinger deleted the split-spring-resource-detectors branch February 28, 2024 12:56
@trask
Copy link
Member

trask commented Feb 28, 2024

I didn't follow why closing, I thought this PR was good?

@laurit
Copy link
Contributor

laurit commented Feb 28, 2024 •
edited
Loading

@trask #10685 will fix the issue in a slightly different way. We'll leave the detectors as they are for the javaagent usage and implement the missing version detection inside the starter (service name detection is already implemented there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask left review comments

@jeanbisutti jeanbisutti jeanbisutti approved these changes

Assignees

@zeitlinger zeitlinger

Labels
spring boot starter test native This label can be applied to PRs to trigger them to run native tests
Projects
No open projects
Spring Boot Starter GA
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Workflow failed: GraalVM native tests (#254)
4 participants
@zeitlinger @trask @laurit @jeanbisutti