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 OTLP protocol configuration options #1880

Merged
merged 9 commits into from
Sep 14, 2021

Conversation

jack-berg
Copy link
Member

Fixes #1879

Changes

Add OTEL_EXPORTER_OTLP_PROTOCOL, OTEL_EXPORTER_OTLP_TRACES_PROTOCOL, and OTEL_EXPORTER_OTLP_METRICS_PROTOCOL environment variables for specifying the OTLP protocol.

@jack-berg jack-berg requested review from a team August 19, 2021 18:52
@carlosalberto
Copy link
Contributor

cc @jsuereth (maybe I'm wrong but I think you were interested in this ;) )

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.

We need to confirm with maintainers that the meaning of these enums i the same across SDKs.

There was concern (previously), that "http/protobuf" and "http/json" are not specific enough in what they mean, and that existing "http/protobuf" or "http/json" exporters in SDKs are actually incompatible with each other.

Opening this as a blocking comments until we have verification from language SDK maintainers that they're comfortable with this as written given the previous attempt to specify this.

@jack-berg
Copy link
Member Author

I've added new commits which revert my first attempt in favor of the approach discussed in the sig meeting of adding new exporter options for the different protocols instead of adding a new configuration option.

@anuraaga anuraaga self-requested a review August 25, 2021 04:32
@anuraaga anuraaga self-requested a review August 25, 2021 23:30
Copy link
Contributor

@tedsuo tedsuo left a comment

Choose a reason for hiding this comment

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

Having now discussed this at leeeeeeeeeength, I'm fine with adding the original proposal.

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks everyone!

@carlosalberto
Copy link
Contributor

Ping @jsuereth

@carlosalberto
Copy link
Contributor

@jsuereth Ping ;) Is your original changes request still standing? Else, let's merge this one.

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.

Think we've left this open with sufficient time for SDK concerns and raised it multiple SiGs. I left ONE comment in the maintainer channel for last-minute discussions.

I don't think this belongs in the specificatoin but taking note here:

Compliance to protocol should be evaluated via Otel Collector ingestion of telemetry.

@carlosalberto
Copy link
Contributor

@jack-berg Oops, looks like we don't have a CHANGELOG entry, please add one ;)

@jack-berg
Copy link
Member Author

All set @carlosalberto!

@carlosalberto carlosalberto merged commit a444093 into open-telemetry:main Sep 14, 2021
Copy link
Member

@alolita alolita left a comment

Choose a reason for hiding this comment

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

+1

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.

Add OTEL_EXPORTER_OTLP_PROTOCOL config option
10 participants