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

Remove ServiceName from OtlpExporterOptions #1557

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Nov 17, 2020

As discussed with @cijothomas, removing ServiceName from OtlpExporterOptions since it is not in the specification.

Opening the PR now to get people's eyes on it with the caveat that I have not yet experimented with things to see what the experience is like when service.name does not get set.

@cijothomas This was recently done by @CodeBlanch in #1420 per an ask from the community. I'd be good with just keeping it, but I can also see the argument that we should push this through the spec. What do you all think?

There is a proposal for an environment configuration open-telemetry/opentelemetry-specification#709.

Changes

Please provide a brief description of the changes here.

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

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@alanwest alanwest requested a review from a team November 17, 2020 01:34
@codecov
Copy link

codecov bot commented Nov 17, 2020

Codecov Report

Merging #1557 (b78411a) into master (68adae6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1557      +/-   ##
==========================================
- Coverage   81.00%   80.98%   -0.02%     
==========================================
  Files         237      237              
  Lines        6453     6449       -4     
==========================================
- Hits         5227     5223       -4     
  Misses       1226     1226              
Impacted Files Coverage Δ
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <ø> (ø)
...try.Exporter.OpenTelemetryProtocol/OtlpExporter.cs 57.89% <100.00%> (-0.65%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️

@CodeBlanch
Copy link
Member

I think if we're going to remove from Otlp, we should also remove from Zipkin & Jaeger. Keep it consistent.

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas
Copy link
Member

I think if we're going to remove from Otlp, we should also remove from Zipkin & Jaeger. Keep it consistent.

Agree. My motivation to have this option removed is:
There is already a well defined Resource attribute from which service-name must be obtained. Providing 2 ways to achieve same thing is always a point of confusion, especially since no formal spec exists on this about which one has higher priority etc.
So lets remove this option from all exporter. Also if the Resoure is not defined, we can fallback to a default like "OpenTelemetry Zipkin" etc.

@cijothomas
Copy link
Member

Opening the PR now to get people's eyes on it with the caveat that I have not yet experimented with things to see what the experience is like when service.name does not get set.

If not set, we currently fallback to a default. We keep the same behavior.

@@ -104,21 +104,6 @@ internal void SetResource(Resource resource)
}
}

if (!processResource.Attributes.Any(kvp => kvp.Key == Resource.ServiceNameKey))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we keep the current behavior of falling back to DefaultServiceName?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok makes sense, wasn't sure about that. Will make the change first thing tomorrow.

@alanwest
Copy link
Member Author

alanwest commented Nov 17, 2020

I think if we're going to remove from Otlp, we should also remove from Zipkin & Jaeger. Keep it consistent.

Oddly, Zipkin actually does specify the ability for configuring a fall back. Jaeger does not.

See:

https://www.github.com/open-telemetry/opentelemetry-specification/tree/master/specification%2Ftrace%2Fsdk_exporters%2Fzipkin.md

Note, exporter to Zipkin MUST allow to configure the default "fall back" name to use as a Zipkin service name.

@cijothomas
Copy link
Member

cijothomas commented Nov 17, 2020

I think if we're going to remove from Otlp, we should also remove from Zipkin & Jaeger. Keep it consistent.

Oddly, Zipkin actually does specify the ability for configuring a fall back. Jaeger does not.

See:

https://www.github.com/open-telemetry/opentelemetry-specification/tree/master/specification%2Ftrace%2Fsdk_exporters%2Fzipkin.md

Note, exporter to Zipkin MUST allow to configure the default "fall back" name to use as a Zipkin service name.

Okay, so lets keep it in Zipkin. Add back to OTLP,Jaeger when spec requires it. For Zipkin, can you also confirm that the Zipkin options one is the fallback one, and Resource is always taken, if available.

Comment on lines +26 to +27
using OpenTelemetry;
using OpenTelemetry.Resources;
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add both OpenTelemetry and OpenTelemetry.Resources here because the AddService extension method is in the OpenTelemetry namespace. Was this intentional? Relates to #1541 in that I think we should aim to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

this requires some changes. Lets merge this PR, and follow up to make it consistent.

Copy link
Member

Choose a reason for hiding this comment

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

@alanwest @cijothomas I put the extensions into OpenTelemetry namespace instead of OpenTelemetry.Resources so that people could use the TracerBuilderExtension.ConfigureResource extension without an additional namespace having to be imported. But that ended up being removed so we can probably move into OpenTelemetry.Resources without any issues, people will just need OpenTelemetry.Trace & OpenTelemetry.Resources in their startup usings (if they want to use ResourceBuilder). Or we put all the builders into OpenTelemetry namespace so people only need that single using in their startup? 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Yes we discussed this in todays SiG meeting as well.. Lets address the overall namespace changes separate from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, good to know the history here. We can continue the namespace conversation separate from this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Doh, it backfired on me! Just updated some code to RC1 and I had to add using OpenTelemetry because of this. I ended up with:

using OpenTelemetry;
using OpenTelemetry.Resources;
using OpenTelemetry.Trace;

All being required to configure TracerProvider w/ Resources in startup code. That sucks. Sorry!

@cijothomas cijothomas merged commit 0e30817 into open-telemetry:master Nov 17, 2020
@alanwest alanwest deleted the alanwest/otlp-exporter-remove-service-name branch March 9, 2021 18:47
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.

3 participants