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

Add sdk-config.yaml starter template w/ references to env vars #76

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Mar 18, 2024

Part of the configuration working group recommendation on how to move forward with #3752 as described here.

  • Part of Define OTEL_EXPERIMENTAL_CONFIG_FILE to ignore other env vars, add env var substitution default syntax opentelemetry-specification#3948
  • Adds a new sdk-migration-config.yaml to the examples directory, which includes env var substitution references to all env vars which map cleanly to file config
  • Adds a new sdk-config.yaml to the example directory, which is identical to sdk-migration-config.yaml, but without env var substitution references
  • Adds a new step to the makefile build which performs env var substitution using js envsub which allows validation to take place after substitution
  • Updates the schema so all types can be null. This ensures a config file is still valid after performing env var substitution with an undefined env var.

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.

LGTM, with fixes on endpoints.

# Configure protocol.
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf}
# Configure endpoint.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:4318/v1/traces}

Copy link
Member

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

Copy link
Member

@marcalff marcalff Mar 18, 2024

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:

Suggested change
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.

Copy link
Member Author

@jack-berg jack-berg Apr 26, 2024

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

# Configure protocol.
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf}
# Configure endpoint.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
Copy link
Member

@marcalff marcalff Mar 18, 2024

Choose a reason for hiding this comment

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

Suggested change
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
endpoint: ${OTEL_EXPORTER_OTLP_METRICS_ENDPOINT:-http://localhost:4318/v1/metrics}

# Configure protocol.
protocol: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http/protobuf}
# Configure endpoint.
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
Copy link
Member

@marcalff marcalff Mar 18, 2024

Choose a reason for hiding this comment

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

Suggested change
endpoint: ${OTEL_EXPORTER_OTLP_PROTOCOL:-http://localhost:-4318}
endpoint: ${OTEL_EXPORTER_OTLP_LOGS_ENDPOINT:-http://localhost:4318/v1/logs}

jack-berg added a commit to open-telemetry/opentelemetry-specification that referenced this pull request Apr 5, 2024
…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.
@jack-berg jack-berg marked this pull request as ready for review April 26, 2024 21:38
@jack-berg jack-berg requested a review from a team as a code owner April 26, 2024 21:38
@jack-berg
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants