-
Notifications
You must be signed in to change notification settings - Fork 896
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 initial env var draft specification #666
Conversation
af73309
to
77a2598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good starting point, some comments.
Have you considered adding a note on OTel language-specific configuration options? Probably mention they are expected to use the same prefix? i.e. Overall looks good, great work! |
In opentelemetry-java PR I proposed more environment variables for OTLP Exporter:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good starting point, we can add more variables later but we should have a starting point.
This should be populated using the OTEL_RESOURCE environment variable
@open-telemetry/specs-approvers This PR documents the current state of implementation. When merged, it will provide a good place for submitting PRs about improving environment variable recommendations. I don't see much controversy during the discussion of this PR, so it seems that we can approve and merge it. |
Approving. A few of the items that will need further refinement (sampling, propagators) already have a related issue, so we can iterate on them after this PR gets merged. |
@carlosalberto Anything else is needed before this PR can be merged? |
Oops, wrong PR comment. For this PR we need at least one approval. @arminru @yurishkuro could be interesting in reviewing this? ;) |
@yurishkuro do you approve this PR now? Anything else should be done here before we can merge it? |
Ping @yurishkuro I think all your issues have been solved ;) |
@open-telemetry/specs-approvers Please review. We have had a few iterations so far, and all things have been addressed (and for the things that are not trivial, there are new, related tickets). For your consideration ;) |
Given that the feedback has been addressed, follow-up issues have been filled, and we have a few approvals (even if not all come from OTel approvers), I will merge this by tomorrow, FYI @open-telemetry/specs-approvers |
Fixes #572.
This is an initial draft of an environment variable specification. As stated in the spec, the goal is to unify the names of environment variables between SDK implementations. SDKs are not required to add configuration for the variables specified, but if they do, they should align with names in this spec.
This list was compiled from comments on #572 and surveying a number of SDK implementations. Defaults have been provided where applicable and likely to be uncontroversial. The door is open to add more detail if we would like to.
Lastly, this is meant to be a starting to point document and align environment variable names going forward. To that end, if a SIG adds a new environment variable that is applicable across languages, it should be documented here. On the same note, if there are environment variables that are more widely applicable that were missed during the survey, add them in the comments and I'll update the PR.