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

support otel.instrumentation.kafka.experimental-span-attributes in spring starter #11263

Merged
merged 5 commits into from
May 15, 2024
Merged

support otel.instrumentation.kafka.experimental-span-attributes in spring starter #11263

merged 5 commits into from
May 15, 2024

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Apr 30, 2024 •
edited
Loading

Depends #11262

@zeitlinger zeitlinger self-assigned this Apr 30, 2024
@zeitlinger zeitlinger requested a review from a team as a code owner April 30, 2024 19:23
@zeitlinger zeitlinger mentioned this pull request Apr 23, 2024
Closed
4 tasks
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Apr 30, 2024
jeanbisutti
jeanbisutti approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

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

@zeitlinger Would it be possible to add an automatic test? Perhaps in another PR?

@zeitlinger
Copy link
Member Author

@zeitlinger Would it be possible to add an automatic test? Perhaps in another PR?

yes, I can add it in #11262

@zeitlinger zeitlinger force-pushed the spring-kafka-experimental-span-attributes branch 2 times, most recently from 79575ac to c8b34e1 Compare May 3, 2024 08:59
@zeitlinger zeitlinger mentioned this pull request May 3, 2024
Merged
@zeitlinger zeitlinger marked this pull request as draft May 8, 2024 06:50
@zeitlinger zeitlinger force-pushed the spring-kafka-experimental-span-attributes branch from c8b34e1 to 4185f83 Compare May 14, 2024 09:33
@zeitlinger zeitlinger mentioned this pull request May 14, 2024
Merged
@zeitlinger zeitlinger added this to the v2.4.0 milestone May 14, 2024
@zeitlinger zeitlinger force-pushed the spring-kafka-experimental-span-attributes branch from 4185f83 to 8271414 Compare May 15, 2024 06:40
@zeitlinger
Copy link
Member Author

@zeitlinger Would it be possible to add an automatic test? Perhaps in another PR?

yes, I can add it in #11262

it's now tested here: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11263/files#diff-23bb7021e8d17c3bda9c13eb1c42019ba2a666e22da6a8d6da97ce4717612546R101

@zeitlinger zeitlinger marked this pull request as ready for review May 15, 2024 10:04
@jeanbisutti jeanbisutti self-requested a review May 15, 2024 12:42
@jeanbisutti jeanbisutti self-requested a review May 15, 2024 13:03
trask
trask approved these changes May 15, 2024
View reviewed changes
...va/io/opentelemetry/instrumentation/spring/autoconfigure/OpenTelemetryAutoConfiguration.java
* Expose the {@link ConfigProperties} bean for use in other auto-configurations.
*
* <p>Why not use spring boot properties directly? <br>
* 1. issues with older spring boot versions <br>
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't these issues affect our (broader) usage of spring boot properties to configure the SDK?

Copy link
Member

Choose a reason for hiding this comment

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

merging to unblock #11350, but i'd be in favor of removing this bullet (or explaining it better), since as written I think it creates more questions than it answers

Copy link
Member Author

@zeitlinger zeitlinger May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it's this style that silently returns false in boot 2

@Value("${otel.instrumentation.common.db-statement-sanitizer.enabled:true}")

https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11350/files#diff-74d5efe463708e7c420646f0b1a8b490c76528e0abba9e8605f6f79bc73e1c40L33

Copy link
Member Author

Choose a reason for hiding this comment

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

wouldn't these issues affect our (broader) usage of spring boot properties to configure the SDK?

no

Copy link
Member

Choose a reason for hiding this comment

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

@zeitlinger

i'd be in favor of removing this bullet (or explaining it better), since as written I think it creates more questions than it answers

Suggested change
* 1. issues with older spring boot versions <br>

Copy link
Member

Choose a reason for hiding this comment

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

(i'll send PR)

@trask trask enabled auto-merge (squash) May 15, 2024 18:11
@trask trask merged commit 5be203a into open-telemetry:main May 15, 2024
53 checks passed
@zeitlinger zeitlinger deleted the spring-kafka-experimental-span-attributes branch May 15, 2024 18:34
@trask trask mentioned this pull request May 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trask trask trask approved these changes

@jeanbisutti jeanbisutti Awaiting requested review from jeanbisutti

Assignees

@zeitlinger zeitlinger

Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zeitlinger @trask @jeanbisutti