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

Prefer arrays of name value pairs over objects #115

Merged
merged 7 commits into from
Sep 19, 2024

Conversation

jack-berg
Copy link
Member

Resolves #72.

May also resolve open-telemetry/opentelemetry-specification#2857.

This PR migrates .resource.attributes and *.otlp.headers to be key-value pairs instead of objects, and adds a modeling rule to follow this pattern for similar situations in the future. To repeat the reasoning from the modeling rule:

When a type requires a configurable list of name-value pairs (i.e. resource attributes, HTTP headers), model using an array of objects, each with name and value properties. While an arrays of name-value objects is slightly more verbose an object where each key-value is an entry, it is preferred because:

  • Avoids user input as keys, which ensures conformity with the snake_case properties rule.
  • Allows both the names and the values to be targets for [env var substitution].

The more I think about it, the more I think this the way the way to go. Note that this is how environment variables are specified in kubernetes manifests, so it shouldn't be particularly foreign to users.

We should get this change in sooner than later to minimize the number of users we expose to churn.

@jack-berg jack-berg requested a review from a team August 30, 2024 19:28
examples/kitchen-sink.yaml Outdated Show resolved Hide resolved
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

See comments.

README.md Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Just a couple of comments, otherwise this looks ok to me

README.md Outdated Show resolved Hide resolved
examples/kitchen-sink.yaml Outdated Show resolved Hide resolved
@jack-berg jack-berg mentioned this pull request Sep 17, 2024
@jack-berg jack-berg requested a review from a team September 18, 2024 15:44
@jack-berg
Copy link
Member Author

@marcalff, @codeboten - I've resolved merge conflicts and incorporated feedback. Can you take another look? Thanks.

@jack-berg jack-berg merged commit f89f35a into open-telemetry:main Sep 19, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants