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

Jaeger exporter service.name clarification needed #1237

Closed
jkwatson opened this issue Nov 18, 2020 · 24 comments · Fixed by #1386
Closed

Jaeger exporter service.name clarification needed #1237

jkwatson opened this issue Nov 18, 2020 · 24 comments · Fixed by #1386
Assignees
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:miscellaneous For issues that don't match any other spec label

Comments

@jkwatson
Copy link
Contributor

What are you trying to achieve?

#1222 includes language that requires the Jaeger exporter to use the service.name attribute of the Resource, but does not explain how the exporter should behave if that attribute is missing from the Resource.

Should the exporter be configured with a fallback service name, or should it fail to export any spans without a valid Resource service.name entry?

@jkwatson jkwatson added the spec:miscellaneous For issues that don't match any other spec label label Nov 18, 2020
@arminru
Copy link
Member

arminru commented Nov 18, 2020

@yurishkuro is there any fallback in Jaeger if it's not provided?
If so, I'd just not set it in that case and we could fix the spec by adding an , if present here:

Critically, Jaeger backend depends on `Span.Process.ServiceName` to identify the service
that produced the spans. That field MUST be populated from the `service.name` attribute
of the [`service` resource](../../resource/semantic_conventions/README.md#service).

@arminru arminru added the area:sdk Related to the SDK label Nov 18, 2020
@yurishkuro
Copy link
Member

Most Jaeger SDKs will refuse to initialize if the service name (a required parameter) is blank, e.g.

I would expect OTEL in general to guarantee that there is always a resource with non-empty service name. In other words, I don't think this should be solved at the level of Jaeger exporters or any Jaeger-specific functionality.

Whether the SDK should raise an exception or default to some unknown_service constant is up for debate.

To unblock Jaeger exporter work specifically, I would suggest going with unknown_service.

@jkwatson
Copy link
Contributor Author

Currently, from what I understand, there is no requirement that service.* is present in the Resource. What is required is that if anything service.* is provided, that service.name is included. [although, I don't know if any SDKs have implemented this particular validation].

The java implementation requires a "service name" to be configured into the Jaeger exporter, but it's not clear how that should interact with the service.name on the Resource (I have guesses about how it should, but it's not specified here).

@yurishkuro
Copy link
Member

The java implementation requires a "service name" to be configured into the Jaeger exporter

That sounds like a bad take. Jaeger backend uses OTEL collector components and can accept OTLP data directly, so the service name must be present in the native OTEL data.

@jkwatson
Copy link
Contributor Author

The java implementation requires a "service name" to be configured into the Jaeger exporter

That sounds like a bad take. Jaeger backend uses OTEL collector components and can accept OTLP data directly, so the service name must be present in the native OTEL data.

But, as I mentioned in the part you didn't quote...there isn't a requirement that the service name is present in the Resource.

@yurishkuro
Copy link
Member

there isn't a requirement that the service name is present in the Resource.

What's stopping us from making it a requirement?

@jkwatson
Copy link
Contributor Author

there isn't a requirement that the service name is present in the Resource.

What's stopping us from making it a requirement?

I don't know. I'm honestly not sure why it isn't a requirement already. That would be a satisfactory resolution to this issue for me.

@yurishkuro
Copy link
Member

I am suggesting making it a requirement, and use unknown_service in the exporter as a fallback, since not all Resource API may be able to enforce required attributes.

@jkwatson
Copy link
Contributor Author

I am suggesting making it a requirement, and use unknown_service in the exporter as a fallback, since not all Resource API may be able to enforce required attributes.

Do you mind championing a spec change for this? I can create the issue if you'd like?

@jkwatson
Copy link
Contributor Author

@yurishkuro #1241

@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Nov 20, 2020
@andrewhsu
Copy link
Member

@tedsuo fyi

@tedsuo
Copy link
Contributor

tedsuo commented Nov 20, 2020

Hey hey! I would also like to see service.name as a required attribute. Almost every tracing system has a version of this which is required.

@yurishkuro
Copy link
Member

I think this should be closed in favor of #1241

@jkwatson
Copy link
Contributor Author

I think this should be closed in favor of #1241

I'd rather keep this open until that one is resolved, since this won't actually be resolved until that one is agreed to and merged.

@jkwatson
Copy link
Contributor Author

So, with #1269 merged, does that mean we should remove the fallback service-name configuration option for the Jaeger exporter and always use whatever is on the Resource? I think 'yes', but it would be good to get a firm confirmation.

@yurishkuro
Copy link
Member

I think fallback configuration should be removed, but it may sense to keep the fallback logic to default to unknown_service if the SDK fails to enforce that. However, my preference would be if the exporter SPI was explicitly defined to require the service name from the SDK at construction.

@carlosalberto
Copy link
Contributor

@jkwatson @yurishkuro My feeling is that with #1294 this should be done, unless you want further clarification, to be on the safe side ;)

@Oberon00
Copy link
Member

I think we want further clarification, which is why I had objected to closing #1241 -- I did not realize this issue here also exists.

@jkwatson
Copy link
Contributor Author

I think our Jaeger exporter spec still needs clarification on this, yes. I wouldn't close this until that document has been updated to explain the 2 different possible resources for the service.name and when to choose which.

@tedsuo
Copy link
Contributor

tedsuo commented Jan 25, 2021

@yurishkuro I'm concerned that we are going to have to drop Jaeger as a v1.0 requirement unless we get the TBD's in the jaeger exporter spec resolved for OTel this week – this is blocking v1.0 at this point. Do you feel like you have enough information to add these attribute translations to the spec? Can we tap a jaeger member of the OTel community to make this PR?

@yurishkuro
Copy link
Member

@tedsuo #1374

@yurishkuro
Copy link
Member

yurishkuro commented Jan 26, 2021

@jkwatson is there still an open issue here? I don't particularly like how the notion of Resource.getDefault is hidden away and there is more than one place that explains something about service.name, but cumulatively those places do seem to define a clear path. It may be worth mentioning it in the jaeger exporter, e.g. "if Resource does not specify service.name, then get it from Resource.getDefault".

@jkwatson
Copy link
Contributor Author

@jkwatson is there still an open issue here? I don't particularly like how the notion of Resource.getDefault is hidden away and there is more than one place that explains something about service.name, but cumulatively those places do seem to define a clear path. It may be worth mentioning it in the jaeger exporter, e.g. "if Resource does not specify service.name, then get it from Resource.getDefault".

heh. I was literally about to write this as well. I do think it would be good to mention that, yes.

@cijothomas
Copy link
Member

For Zipkin also: #1380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:miscellaneous For issues that don't match any other spec label
Projects
None yet
8 participants