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 sdk-config.yaml starter template w/ references to env vars #76
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM, with fixes on endpoints.
examples/sdk-config.yaml
Outdated
# Configure protocol. | ||
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf} | ||
# Configure endpoint. | ||
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} |
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.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} | |
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:4318/v1/traces} |
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.
That would make it OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
Also, its currently using OTEL_EXPORTER_OTLP_PROTOCOL
when I'm guessing it wants either OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
or to actually put the path outside the environment variable default to apply to both:
endpoint: ${OTEL_EXPORTER_OTLP_ENDPOINT:-http://localhost:4318}/v1/traces
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.
Good point on the TRACES part. So, something like this then:
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} | |
endpoint: ${OTEL_EXPORTER_OTLP_TRACES_ENDPOINT:-http://localhost:4318/v1/traces} |
This also means using the TRACES env var for PROTOCOL, CERTIFICATES, CLIENT_KEY, etc as well.
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.
We have to decide between the generic version of these properties and the signal specific. I think the generic version of these is likely to be more popular. But the generic version of the property doesn't require you to configure a path.
One thing which is still ambiguous is whether the endpoints specified in file config have paths appended automatically if the protocol is http/protobuf
and the path is omitted. I lean towards yes.
If the answer is no, then we should go back and update the other examples, which currently reference http://localhost:4318
without any path: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L64
examples/sdk-config.yaml
Outdated
# Configure protocol. | ||
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf} | ||
# Configure endpoint. | ||
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} |
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.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} | |
endpoint: ${OTEL_EXPORTER_OTLP_METRICS_ENDPOINT:-http://localhost:4318/v1/metrics} |
examples/sdk-config.yaml
Outdated
# Configure protocol. | ||
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf} | ||
# Configure endpoint. | ||
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} |
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.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318} | |
endpoint: ${OTEL_EXPORTER_OTLP_LOGS_ENDPOINT:-http://localhost:4318/v1/logs} |
…v var substitution default syntax (#3948) Fixes #3752. Implementation of the configuration working group recommendation as [described here](#3752 (comment)). - Adds definition for `OTEL_EXPERIMENTAL_CONFIG_FILE `, which ignores other env vars when evaluating, except env var substitution references - Adds new env var substitution default syntax `${ENVVAR:-defaultValue}` - Paired with open-telemetry/opentelemetry-configuration#76, which defines a new [sdk-config.yaml](https://github.com/jack-berg/opentelemetry-configuration/blob/starter-template/examples/sdk-config.yaml) starter template referencing all env vars that map cleanly.
d05a637
to
73cd03f
Compare
This is a loose end from open-telemetry/opentelemetry-specification#3948 I've pushed an additional commit and marked as ready for review. Please review when you have the chance. cc @open-telemetry/configuration-maintainers |
Part of the configuration working group recommendation on how to move forward with #3752 as described here.
sdk-migration-config.yaml
to the examples directory, which includes env var substitution references to all env vars which map cleanly to file configsdk-config.yaml
to the example directory, which is identical tosdk-migration-config.yaml
, but without env var substitution references