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

Jump to bottom

Add OTEL_EXPORTER_ZIPKIN_TRANSPORT_FORMAT to sdk-environment-variables #1340

Closed
wants to merge 4 commits into from
Closed

Add OTEL_EXPORTER_ZIPKIN_TRANSPORT_FORMAT to sdk-environment-variables #1340

wants to merge 4 commits into from

Conversation

srikanthccv
Copy link
Member

Fixes #1336

Changes

This change adds environment variable to specify transport format for Zipkin exporter with default value V2_PROTOBUF.

@srikanthccv srikanthccv requested review from a team January 14, 2021 06:18
iNikem
iNikem reviewed Jan 19, 2021
View reviewed changes
specification/sdk-environment-variables.md
| Name | Description | Default |
| ----------------------------- | -------------------------- | ------------------------------------------------------------------------------------------------------------ |
| OTEL_EXPORTER_ZIPKIN_ENDPOINT | Endpoint for Zipkin traces | <!-- markdown-link-check-disable --> "http://localhost:9411/api/v2/spans"<!-- markdown-link-check-enable --> |
| OTEL_EXPORTER_ZIPKIN_TRANSPORT_FORMAT | Transport format for Zipkin traces | V2_PROTOBUF |
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that all language SDKs have to support all these formats? Do they already? If not, how much work will it be to add this before GA?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems certain language SDKs already have support for some formats and not sure about the progress on others but everything mentioned here is already part of spec-compliance-matrix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @iNikem 's question is still a good one. Are all languages required to implement all protocols and transport formats in order to get to a 1.0 release? I don't think so, but if so, that's a very significant change to what maintainers believe are required for 1.0.

@jsuereth
Copy link
Contributor

We discussed this in the Specifcation SiG. I'm capturing the resulting decisions/actions from that.

  • Adding this environment variable would be "allowed-for-GA" but should not be done in 1.0 unless we can verify SDK compliance in time for 1.0.
  • There will be no specified default for format on SDKs. This means no change for 1.0 of trace is required, but there is still room to add this environment variable post-1.0 (if we can't get tthe change in before 1.0 in all SDKs). Effectively, this change will be ok to add post 1.0.
  • We will also remove the specification for the OTLP exporter format to be consistent with the decision on ZIPKIN here for 1.0.
  • At a minimum, SDKs should already be documenting Zipkin exporter usage (and the format that is default), outside the specification.

I'll be opening a specification PR to update the OTLP exporter specification.

jsuereth added a commit to jsuereth/opentelemetry-specification that referenced this pull request Jan 20, 2021
@jsuereth
Adapt open-telemetry#1340 for 1.0 of Trace.
9bef47c 
- Ensure environment variables have consistent names
- Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
@jsuereth jsuereth mentioned this pull request Jan 20, 2021
Merged
Base automatically changed from master to main January 27, 2021 21:16
jsuereth added a commit to jsuereth/opentelemetry-specification that referenced this pull request Jan 28, 2021
@jsuereth
Adapt open-telemetry#1340 for 1.0 of Trace.
d751874 
- Ensure environment variables have consistent names
- Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
jsuereth added a commit to jsuereth/opentelemetry-specification that referenced this pull request Jan 29, 2021
@jsuereth
OAdapt open-telemetry#1340 for 1.0 of Trace.
6a30cf2 
- Ensure environment variables have consistent names
- Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
carlosalberto pushed a commit that referenced this pull request Feb 3, 2021
@jsuereth
Modify Exporter format variables for 1.0 (allowing further specificat…
61811e1 
…ion post 1.0) (#1358)

* OAdapt #1340 for 1.0 of Trace.
- Ensure environment variables have consistent names
- Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 4, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 11, 2021
@carlosalberto carlosalberto reopened this Feb 12, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Feb 20, 2021
@carlosalberto carlosalberto reopened this Feb 22, 2021
@carlosalberto
Copy link
Contributor

Re-opening again. I think it's about time we start working on it ;)

@srikanthccv
Copy link
Member Author

@carlosalberto I see that OTEL_EXPORTER_ZIPKIN_PROTOCOL is added in #1358. Is this still relevant?

@carlosalberto
Copy link
Contributor

@lonewolf3739 It has been reserved, but not defined yet, so yes, we definitely to work on that (non an urgent matter, although worth keeping it in mind).

@github-actions
Copy link

github-actions bot commented Mar 2, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 2, 2021
@seemk
Copy link
Contributor

seemk commented Mar 2, 2021 •
edited
Loading

I think a similar env var, OTEL_EXPORTER_JAEGER_TRANSPORT_FORMAT, is needed for Jaeger as well to allow configuring between protobuf and thrift. Although not part of this PR, just a side note :)

Edit: perhaps OTEL_EXPORTER_ZIPKIN_PROTOCOL as added in #1358 would indeed be better and it's already used in the Python SDK. Will need a similar env var for Jaeger though.

@github-actions github-actions bot removed the Stale label Mar 3, 2021
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 11, 2021
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 18, 2021
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jsuereth
Modify Exporter format variables for 1.0 (allowing further specificat…
7aaaecf 
…ion post 1.0) (open-telemetry#1358)

* OAdapt open-telemetry#1340 for 1.0 of Trace.
- Ensure environment variables have consistent names
- Update documentation to leave format UNSPECIFIED in 1.0 with an allowance to add config later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkwatson jkwatson jkwatson left review comments

@reyang reyang reyang approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@iNikem iNikem iNikem approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees

@jsuereth jsuereth

@bogdandrutu bogdandrutu

Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Environment variable for zipkin exporter transport format
10 participants
@srikanthccv @jsuereth @carlosalberto @seemk @anuraaga @jkwatson @iNikem @tigrannajaryan @reyang @bogdandrutu