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

make spring boot honor the standard environment variables for maps #11000

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

zeitlinger
Copy link
Member

Fixes #10959

@zeitlinger zeitlinger self-assigned this Apr 2, 2024
@zeitlinger zeitlinger requested a review from a team as a code owner April 2, 2024 11:03
@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 2, 2024
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.

The map properties defined with an environment variable seems to take precedence over the map properties defined from a .properties / .yml file? It may be worth to document this in the next release.

@zeitlinger
Copy link
Member Author

The map properties defined with an environment variable seems to take precedence over the map properties defined from a .properties / .yml file? It may be worth to document this in the next release.

Spring also gives precedence to environment variables, so I think spring users will expect that.

@jeanbisutti
Copy link
Member

Spring also gives precedence to environment variables, so I think spring users will expect that.

The possibility of overriding some map values with an environment variable could be documented.

@zeitlinger
Copy link
Member Author

Spring also gives precedence to environment variables, so I think spring users will expect that.

The possibility of overriding some map values with an environment variable could be documented.

I'd argue this is standard spring behavior, but I'm happy to point that out in the docs.

I'll link the PR here once done.

@zeitlinger
Copy link
Member Author

I'll link the PR here once done.

open-telemetry/opentelemetry.io#4241

* If you specify the environment variable <code>OTEL_RESOURCE_ATTRIBUTES_POD_NAME</code>, then
* Spring Boot will ignore <code>OTEL_RESOURCE_ATTRIBUTES</code>, which violates the principle of
* least surprise. This method merges the two maps, giving precedence to <code>
* OTEL_RESOURCE_ATTRIBUTES_POD_NAME</code>, which is more specific and which is also the value
Copy link
Member

Choose a reason for hiding this comment

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

the env var mapping makes me a little nervous since some otel resource attribute names include underscores

but I think we can argue that this is just a spring configuration feature(?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't follow - what are you nervous about? Maybe an example would help.

Copy link
Member

Choose a reason for hiding this comment

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

some OTel resource attributes have underscores in them, and so it's not clear to me how these would work with env vars, e.g. does OTEL_RESOURCE_ATTRIBUTES_X_Y_Z correspond to resource attribute x.y.z, x.y_z, x_y.z, or x_y_z?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, now I get it!

Based on that, I'd say:

  • OTEL_RESOURCE_ATTRIBUTES_X_Y_Z should only be used to update an x.y.z or x_y_z that you have defined in application.yaml
  • use OTEL_RESOURCE_ATTRIBUTES if you want to introduce new resource attributes

I think open-telemetry/opentelemetry.io#4241 is the right place to elaborate on this distinction.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, OTEL_RESOURCE_ATTRIBUTES_X_Y_Z results in x.y.z - which is indeed a spring boot feature.

Copy link
Member

@trask trask Apr 8, 2024

Choose a reason for hiding this comment

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

OTEL_RESOURCE_ATTRIBUTES_X_Y_Z should only be used to update an x.y.z or x_y_z that you have defined in application.yaml

oh, so it will auto-match if you have something matching already defined in application.yaml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it will auto match.

@trask trask merged commit 19b65c0 into open-telemetry:main Apr 8, 2024
49 checks passed
@zeitlinger zeitlinger deleted the merge-spring-maps branch April 10, 2024 08:13
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
3 participants