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

Support env variable configuration for OTLP exporter #1004

Closed
codeboten opened this issue Aug 17, 2020 · 8 comments · Fixed by #1101
Closed

Support env variable configuration for OTLP exporter #1004

codeboten opened this issue Aug 17, 2020 · 8 comments · Fixed by #1101
Assignees

Comments

@codeboten
Copy link
Contributor

The spec describes environment variables that should be supported to configure the OTLP exporter, this feature request is to add support in the current implementation.

Spec PR:
https://github.com/open-telemetry/opentelemetry-specification/pull/699/files

@jan25
Copy link
Contributor

jan25 commented Aug 23, 2020

@codeboten can i help fix this?

@jan25
Copy link
Contributor

jan25 commented Aug 25, 2020

Looks like current OTLP exporter code doesn't cover all the cases covered in spec.

These seem like can be immediately supported:

  • OTEL_EXPORTER_OTLP_SPAN_ENDPOINT OTEL_EXPORTER_OTLP_METRIC_ENDPOINT
  • OTEL_EXPORTER_OTLP_SPAN_INSECURE OTEL_EXPORTER_OTLP_METRIC_INSECURE
  • OTEL_EXPORTER_OTLP_SPAN_CERTIFICATE OTEL_EXPORTER_OTLP_METRIC_CERTIFICATE

These require the cases to be implemented(for e.g. moving to trace/metric experimental API) before adding support for ENV based configuration:

  • OTEL_EXPORTER_OTLP_SPAN_PROTOCOL OTEL_EXPORTER_OTLP_METRIC_PROTOCOL
  • OTEL_EXPORTER_OTLP_SPAN_HEADERS OTEL_EXPORTER_OTLP_METRIC_HEADERS
  • OTEL_EXPORTER_OTLP_SPAN_COMPRESSION OTEL_EXPORTER_OTLP_METRIC_COMPRESSION
  • OTEL_EXPORTER_OTLP_SPAN_TIMEOUT OTEL_EXPORTER_OTLP_METRIC_TIMEOUT

Would you recommend we cover the top 3 cases and have a way to work with ENV variables, and for later cases the support can be added as they are implemented?

@owais
Copy link
Contributor

owais commented Aug 25, 2020

I think add env var based support to existing options makes sense. Adding new the missing options can be a different stream of work.

@codeboten
Copy link
Contributor Author

I agree, supporting the ones that are already present seems like a good first step. Also worth implementing the case where a general config option is set instead of SPAN/METRIC specific ones:

OTEL_EXPORTER_OTLP_ENDPOINT
OTEL_EXPORTER_OTLP_INSECURE
OTEL_EXPORTER_OTLP_CERTIFICATE
OTEL_EXPORTER_OTLP_HEADERS

Worth noting that the current OTLP exporter calls headers metadata: https://github.com/open-telemetry/opentelemetry-python/blob/master/exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py#L110

@jan25
Copy link
Contributor

jan25 commented Sep 13, 2020

I opened a WIP PR. Any feedback early on would be helpful :)

@jan25
Copy link
Contributor

jan25 commented Sep 13, 2020

Also, I think we can clarify few things in exporter spec:

  • Mention that OTEL_EXPORTER_OTLP_CERTIFICATE and related vars take path to certificate file.
  • Clarify on format of OTEL_EXPORTER_OTLP_HEADERS . For e.g. is it JSON string?
  • Add sample examples for above flags in same spec file.
  • OTEL_EXPORTER_OTLP_INSECURE can take true for default. That way supplying certificates is 'not' mandatory.

wdyt?

@codeboten
Copy link
Contributor Author

Sure, I think it makes sense to clarify this in the spec. I would expect the format for OTLP_HEADERS to be a list of key/value pairs like the the resource attributes env variable: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md#general-sdk-configuration

@jan25
Copy link
Contributor

jan25 commented Sep 17, 2020

Cool. I'll find some time soon to open a PR in spec repo to discuss more and also to finalize this current issue

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 a pull request may close this issue.

3 participants