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

add support for missing list properties in spring starter #12434

Merged
merged 10 commits into from
Oct 16, 2024

Conversation

zeitlinger
Copy link
Member

@zeitlinger zeitlinger commented Oct 11, 2024

Spring needs special handling of lists and maps

  • yaml lists only work if you create a @ConfigurationProperties object
  • strings and other primitives don't need @ConfigurationProperties - you can simply get them from the environment

So what was actually missing:

@zeitlinger zeitlinger self-assigned this Oct 11, 2024
@zeitlinger zeitlinger requested a review from a team as a code owner October 11, 2024 09:12
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Oct 11, 2024
@zeitlinger
Copy link
Member Author

@jeanbisutti please take a look

@jeanbisutti
Copy link
Member

Does this PR mean that the list properties were not properly managed by the OTel starter?

SpringConfigProperties has already a getList implementation:

What are the limitations of the current implementation?

@zeitlinger
Copy link
Member Author

What are the limitations of the current implementation?

not all relevant list properties were included - and all list properties have to be supported specifically due to how spring treats lists

@jeanbisutti
Copy link
Member

not all relevant list properties were included

This one seems to be new:

https://github.com/zeitlinger/opentelemetry-java-instrumentation/blob/3952fcfd785f7eb9cc8ef4e75fd59f99059a41fd/instrumentation/spring/spring-boot-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json#L80

and all list properties have to be supported specifically due to how spring treats lists

Could you please elaborate this and the new properties in the PR description? It would help to review the PR and the users understand what is new.

@zeitlinger
Copy link
Member Author

Could you please elaborate this and the new properties in the PR description? It would help to review the PR and the users understand what is new.

done

zeitlinger and others added 3 commits October 15, 2024 16:33
…/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/OtelSpringProperties.java

Co-authored-by: Jean Bisutti <jean.bisutti@gmail.com>
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.

LGTM

Thanks @zeitlinger!

@trask trask merged commit 9e83898 into open-telemetry:main Oct 16, 2024
56 checks passed
@zeitlinger zeitlinger deleted the spring-properties branch October 17, 2024 06:20
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
Development

Successfully merging this pull request may close these issues.

5 participants