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.

Sign up for GitHub

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

RFC | add more environment variables to OTEL exporter #106

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

RFC | add more environment variables to OTEL exporter #106

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
146e65d
add more environment variables to OTEL exporter
hgiasac Feb 21, 2024
00793c0
add other OTEL config guideline
hgiasac Feb 21, 2024
e3ae046
remove redundant OTEL_EXPORTER_ prefix
hgiasac Feb 21, 2024
0235137
clarify the required propagation
hgiasac Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions rfcs/0000-deployment.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,15 @@ _This RFC does not specify the following planned changes:_
- The connector can read environment variables on startup for configuration purposes
- The following environment variables are reserved, and should not be used for connector-specific configuration:
- `HASURA_*`
- `OTEL_EXPORTER_*`
- `OTEL_*`
- Connectors can use environment variables as part of their configuration. Configuration that varies between different environments or regions (like connection strings) should be configurable via environment variables.
- The connector should send any relevant trace spans in the OTLP format to the OTEL collector hosted at the URL provided by the `OTEL_EXPORTER_OTLP_ENDPOINT` environment variable.
- Note: `OTEL_EXPORTER_OTLP_ENDPOINT` indicates the _root URL_ of the collector, and not the `/v1/traces` endpoint.
- Spans should indicate the service name provided by the `OTEL_SERVICE_NAME` environment variable.
- The connector can configure additional headers to add in outgoing gRPC or HTTP requests by the `OTEL_EXPORTER_OTLP_HEADERS` environment variables.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not the connector which configures these though, right? It's the user of the Docker image?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The connector presumably just needs to respect the OTEL spec and pass them along?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. My words may cause confusing here

- The connector also can configure additional resource attributes for requests by the `OTEL_RESOURCE_ATTRIBUTES` environment variables.
- Note: the value of `OTEL_EXPORTER_OTLP_HEADERS` and `OTEL_RESOURCE_ATTRIBUTES` is key-value pairs, for example, `api-key=key,other-config-value=value`.
- Other OTLP exporter configurations are optional. However, they must follow the [Environment Variable Specification](https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables) and [SDK config](https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to be a bit careful here because the environment variable spec says too much IMO. For example, it requires OTEL_PROPAGATORS, but the NDC spec requires tracecontext propagation. There are a lot of other env vars that we don't need to support in full.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a line to clarify the required tracecontext propagation.

I'm wondering if we should specify a list of individual env vars to support, or to just say "a connector should support OTEL in full" but clarify which parts we actually care about right now.

IMO not all languages implement OTEL SDK. It seems too much to expect the author to support OTEL in full. However, we can clarify that the connector can support other configurations depending on the underlying SDK.

Anyway, it would be great if you could suggest better words to make the proposal clearer.

- If the `HASURA_SERVICE_TOKEN_SECRET` environment variable is specified and non-empty, then the connector should implement bearer-token HTTP authorization using the provided static secret token.
- Information log messages should be logged in plain text to standard output.
- Error messages should be logged in plain text to standard error.
Expand All @@ -45,10 +49,6 @@ _This RFC does not specify the following planned changes:_
- To support these build steps, tooling should support building images from Dockerfiles. See "Deployment API" below.
- The motivation is that we may want to provision a connector process on short notice, e.g. to serve an incoming request.

### Open Questions

- Do we want to reserve environment variables `OTEL_*` for possible future use of the [OTLP exporter spec](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md)?

## Deployment API

Docker images should be built in the same environment as which they are run, to avoid possible issues with differences in architecture, etc. Therefore, we need to specify _how to build_ images which meet the specification above.
Expand Down
Loading