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

add support for missing spring list properties #12819

Merged
merged 7 commits into from
Dec 12, 2024
Merged

add support for missing spring list properties #12819

merged 7 commits into from
Dec 12, 2024

Conversation

zeitlinger
Copy link
Member

No description provided.

@zeitlinger zeitlinger self-assigned this Nov 29, 2024
@zeitlinger zeitlinger requested a review from a team as a code owner November 29, 2024 15:57
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Nov 29, 2024
zeitlinger
zeitlinger commented Nov 29, 2024
View reviewed changes
...boot-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json
@@ -419,8 +419,8 @@
},
{
"name": "otel.instrumentation.logback-appender.experimental.capture-mdc-attributes",
"type": "java.util.List<java.lang.String>",
"description": "MDC attributes to capture. Use the wildcard character <code>*</code> to capture all attributes."
"type": "java.lang.String",
Copy link
Member Author

Choose a reason for hiding this comment

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

@jeanbisutti this property is used by

opentelemetry-java-instrumentation/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java

Line 19 in 5b21872

class LogbackAppenderInstaller {
- and I don't know if we can access @CofigurationProperties at that stage yet

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean.

From what I observe, @Value does not seem to work in LogbackAppenderApplicationListener.

org.springframework.boot.context.logging.LoggingApplicationListener does not use @Value .

@zeitlinger
Copy link
Member Author

@jeanbisutti please review

jeanbisutti
jeanbisutti reviewed Dec 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.

To help make the release note, could you please list the added properties in the description?

Are we sure that all the added properties are working? Perhaps it may be worth adding automatic tests.

...boot-autoconfigure/src/main/resources/META-INF/additional-spring-configuration-metadata.json
@@ -419,8 +419,8 @@
},
{
"name": "otel.instrumentation.logback-appender.experimental.capture-mdc-attributes",
"type": "java.util.List<java.lang.String>",
"description": "MDC attributes to capture. Use the wildcard character <code>*</code> to capture all attributes."
"type": "java.lang.String",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean.

From what I observe, @Value does not seem to work in LogbackAppenderApplicationListener.

org.springframework.boot.context.logging.LoggingApplicationListener does not use @Value .

@zeitlinger zeitlinger added this to the v2.11.0 milestone Dec 12, 2024
@zeitlinger
Copy link
Member Author

Perhaps it may be worth adding automatic tests.

done

@laurit
Copy link
Contributor

laurit commented Dec 12, 2024

@zeitlinger could you elaborate what this change is about. Did these list properties work at all previously or did the work in some different way? If the worked in some different way is this change backwards compatible?

@zeitlinger
Copy link
Member Author

Did these list properties work at all previously or did the work in some different way?

Old:

prop: a,b

New:

prop: a,b

OR

prop: [a,b]

If the worked in some different way is this change backwards compatible?

yes

@laurit laurit merged commit 61b8f72 into open-telemetry:main Dec 12, 2024
56 checks passed
@zeitlinger zeitlinger deleted the list-properties branch January 13, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurit laurit laurit approved these changes

@jeanbisutti jeanbisutti jeanbisutti approved these changes

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.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@zeitlinger @laurit @jeanbisutti