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

Define the fallback case for service.name #1269

Merged

Conversation

carlosalberto
Copy link
Contributor

Fixes #1241, #1237

This is a very minimalistic clarification based on what currently many SIGs do (providing a fallback based on the running service), finally defaulting to the unknown_service literal.

@carlosalberto carlosalberto requested review from a team December 1, 2020 05:44
@carlosalberto carlosalberto added area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:resource Related to the specification/resource directory priority:p2 Medium priority level labels Dec 1, 2020
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

In my opinion, this is the wrong place to specify this. I mean, it's fine to additionally specify it here, but we need a list of required resources in the SDK specification. Maybe https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/sdk.md, although in principle it applies to tracing, metrics and probably logging SDK.
I think we should not assume that SDK implementers will read all resource semantic conventions.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor Author

I think we should not assume that SDK implementers will read all resource semantic conventions.

Fair point. I think this documentation needs to be here, but we also need to mention this in the main sdk.md document, with a link to the specific case of service.name, as this is a rather important item.

@Oberon00
Copy link
Member

Oberon00 commented Dec 1, 2020

We probably also want that for the telemetry.sdk resources BTW.

specification/resource/sdk.md Outdated Show resolved Hide resolved
specification/resource/sdk.md Outdated Show resolved Hide resolved
carlosalberto and others added 2 commits December 2, 2020 23:49
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@andrewhsu
Copy link
Member

@open-telemetry/specs-approvers should this really be P1 since the linked issues #1241, #1237 are also P1?

@jmacd
Copy link
Contributor

jmacd commented Dec 3, 2020

Based on this comment, #1241 (comment), I'm inclined to promote my own suggestion above:

#1269 (comment)

Explicitly, it means that we can remove some of the words in this PR and simply specify one of the two options in my comment.

@austinlparker
Copy link
Member

Just as a note, I think whatever the decision is here needs to be something that is obvious to an operator/consumer of the telemetry and something that is non-suitable for actual usage (such as a 'smart' derived name based on command line or whatnot). Ultimately the name of a service is a logical decision made by humans who are interpreting data, but if we're too clever about it, some users may defer to the cleverness which is good up until the moment it isn't. An obvious default that isn't suitable for long-term usage satisfies both of these conditions.

@jmacd
Copy link
Contributor

jmacd commented Dec 3, 2020

The semantic conventions in https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/process.md already contain the command name, and the telemetry SDK details are built in (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/resource/semantic_conventions/README.md), so based on @austinlparker's comment I think when service.name is not set exporters MUST use opentelemetry_service as a fallback.

@carlosalberto
Copy link
Contributor Author

After a few iterations myself, I think we either go with:

  1. Something standard based on the semantic conventions (as @jmacd proposed), such as process.executable.name or process.command - with a fallback to opentelemetry_service or unknown_service, OR
  2. Fallback directly to opentelemetry_service or unknown_service.

Opinions? cc @yurishkuro @Oberon00

@yurishkuro
Copy link
Member

I prefer "unknown" to be in the name. I don't have a strong opinion on auto-resolvers, but I share Austin's concern with them not doing the right thing and being worse than the default.

@jmacd
Copy link
Contributor

jmacd commented Dec 4, 2020

I support "unknown"

@carlosalberto
Copy link
Contributor Author

Ping @Oberon00 (as I'm also slightly inclined to use unknown_service without the fallback, for the operators interaction that Austin mentioned).

@Oberon00
Copy link
Member

Oberon00 commented Dec 5, 2020

"unknown" is fine. Or "unknown:EXE-NAME".

@carlosalberto
Copy link
Contributor Author

Ping @Oberon00 @austinlparker

@austinlparker
Copy link
Member

lgtm

specification/resource/semantic_conventions/README.md Outdated Show resolved Hide resolved
specification/resource/semantic_conventions/README.md Outdated Show resolved Hide resolved
specification/resource/semantic_conventions/README.md Outdated Show resolved Hide resolved
@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA and removed priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs labels Dec 11, 2020
carlosalberto and others added 2 commits December 11, 2020 17:44
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@Oberon00 Oberon00 dismissed their stale review December 11, 2020 19:11

I think there is awareness now for the problem with #1034, so not blocking this PR (trusting you to sort this out properly)

@carlosalberto
Copy link
Contributor Author

I think there is awareness now for the problem with #1034, so not blocking this PR (trusting you to sort this out properly)

Thanks! Have made service.instance.id non-required, so this can be properly addressed in #1034

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:resource Related to the specification/resource directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service name should be mandatory for all configured resources.
7 participants