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

Make Otlp exporter configuration environment variables specs-compliant #974

Merged
merged 10 commits into from
Sep 14, 2021

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Sep 8, 2021

Fixes #971

Changes

This PR makes otlp exporter configuration environment variables specs compliant.
Also add one new env variable ( not part of specs ) - OTEL_EXPORTER_OTLP_CERTIFICATE_STRING to specify certificate as string.

Also introduce new env-variables ( not part of specs ) `

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@lalitb lalitb requested a review from a team September 8, 2021 21:31
@lalitb lalitb changed the title add documentation for GRPC Env variables add documentation for GRPC configuration Sep 8, 2021
@codecov
Copy link

codecov bot commented Sep 8, 2021

Codecov Report

Merging #974 (c192b58) into main (0da25b3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #974   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files         161      161           
  Lines        6817     6817           
=======================================
  Hits         6502     6502           
  Misses        315      315           

exporters/otlp/README.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Tan <lilotom@gmail.com>
exporters/otlp/README.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Tan <lilotom@gmail.com>
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Commented on the bug as well.

I don't think this is sufficient for a 1.0 against the specification.

I think there are two enviornment variables here that you can leave for legacy users BUT you should provide the new environment variables as alternatives, specifically:

OTEL_EXPORTER_OTLP_TRACES_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE

@lalitb
Copy link
Member Author

lalitb commented Sep 9, 2021

@jsuereth - It was creating more noise in code to support both legacy and compliant environment variables, and would further increase in future once we start supporting OTEL_EXPORTER_OTLP_[TRACES_]PROTOCOL. Keeping that in mind, have removed legacy environment variables. This would be breaking change ( hopefully won't affect lots of users ), and we will document this in release description.

@@ -59,6 +59,7 @@ std::unique_ptr<proto::collector::trace::v1::TraceService::Stub> MakeServiceStub
const OtlpGrpcExporterOptions &options)
{
std::shared_ptr<grpc::Channel> channel;

Copy link
Member Author

@lalitb lalitb Sep 9, 2021

Choose a reason for hiding this comment

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

will remove this commit later.

@lalitb lalitb changed the title add documentation for GRPC configuration Make Otlp exporter configuration environment variables specs-compliant Sep 9, 2021
@lalitb
Copy link
Member Author

lalitb commented Sep 13, 2021

@owent - As these env variables were initially modified by you, would like to have a look into this PR. Thanks.

@owent
Copy link
Member

owent commented Sep 14, 2021

@owent - As these env variables were initially modified by you, would like to have a look into this PR. Thanks.

LGTM.

Should we also make the same changes for OtlpHttpExporter ?

@lalitb
Copy link
Member Author

lalitb commented Sep 14, 2021

LGTM.

Should we also make the same changes for OtlpHttpExporter ?

Thanks for reviewing @owais. OtlpHttpExporter doesn't support environment variables yet, so we are not non-compliant there :) Will create a github issue for adding env variables there.

@owent
Copy link
Member

owent commented Sep 14, 2021

LGTM.
Should we also make the same changes for OtlpHttpExporter ?

Thanks for reviewing @owais. OtlpHttpExporter doesn't support environment variables yet, so we are not non-compliant there :) Will create a github issue for adding env variables there.

OK. I think I can add support of these environment variables for OtlpHttpExporter then:

  • OTEL_EXPORTER_OTLP_ENDPOINT
  • OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
  • OTEL_EXPORTER_OTLP_HEADERS
  • OTEL_EXPORTER_OTLP_TRACES_HEADERS
  • OTEL_EXPORTER_OTLP_TIMEOUT
  • OTEL_EXPORTER_OTLP_TRACES_TIMEOUT

But OTEL_EXPORTER_OTLP_CERTIFICATE and OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE can be available only after #938 is finished.

@lalitb
Copy link
Member Author

lalitb commented Sep 14, 2021

OK. I think I can add support of these environment variables for OtlpHttpExporter then:

  • OTEL_EXPORTER_OTLP_ENDPOINT
  • OTEL_EXPORTER_OTLP_TRACES_ENDPOINT
  • OTEL_EXPORTER_OTLP_HEADERS
  • OTEL_EXPORTER_OTLP_TRACES_HEADERS
  • OTEL_EXPORTER_OTLP_TIMEOUT
  • OTEL_EXPORTER_OTLP_TRACES_TIMEOUT

That would be awesome :)

But OTEL_EXPORTER_OTLP_CERTIFICATE and OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE can be available only after #938 is finished.

Agree.

@ThomsonTan ThomsonTan merged commit 43d8fe4 into open-telemetry:main Sep 14, 2021
@lalitb lalitb mentioned this pull request Sep 17, 2021
3 tasks
@lalitb lalitb mentioned this pull request Mar 22, 2022
3 tasks
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.

OTLP Environment variables do not match specification
4 participants