Add OTEL_SERVICE_NAME environment variable
#1677
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ For example, the value `12000` indicates 12000 milliseconds, i.e., 12 seconds. | |
| | Name | Description | Default | Notes | | ||
| | ------------------------ | ------------------------------------------------- | --------------------------------- | ----------------------------------- | | ||
| | OTEL_RESOURCE_ATTRIBUTES | Key-value pairs to be used as resource attributes | | See [Resource SDK](./resource/sdk.md#specifying-resource-information-via-an-environment-variable) for more details. | | ||
| | OTEL_SERVICE_NAME | Sets the value of the [`service.name`](./resource/semantic_conventions/README.md#service) resource attribute | | If `service.name` is also provided in `OTEL_RESOURCE_ATTRIBUTES` then `OTEL_SERVICE_NAME` takes precedence. | | ||
iNikem marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Should we also add There was a problem hiding this comment. I usually prefer to keep PR scope small, if possible. This PR solves one specific issue. I am happy to file a follow-up one, if you feel this is needed :) There was a problem hiding this comment. That's reasonable. If we add it, it would ideally be in the same spec version, however, so SDK maintainers/implementers can tackle this tightly coupled feature at once. The next release is scheduled for early June, so we should be good here, as the follow-up should be as uncontroversial as this PR. There was a problem hiding this comment. Same as #1677 (comment): I don't think the service version is used often enough to warrant it's own attribute. We did receive user requests for service.name AFAIK, but not for any others. |
||
| | OTEL_LOG_LEVEL | Log level used by the SDK logger | "info" | | | ||
| | OTEL_PROPAGATORS | Propagators to be used as a comma separated list | "tracecontext,baggage" | Values MUST be deduplicated in order to register a `Propagator` only once. | | ||
| | OTEL_TRACES_SAMPLER | Sampler to be used for traces | "parentbased_always_on" | See [Sampling](./trace/sdk.md#sampling) | | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add NAMESPACE and INSTANCE as well. Service name should not be treated special than the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually prefer to keep PR scope small, if possible. This PR solves one specific issue. I am happy to file a follow-up one, if you feel this is needed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never heard of anyone using namespace (but maybe it just works so well that there are no questions?). instance.id has is currently IMHO too vaguely defined to be useful, as discussed in #1034. So the service name is IMHO definitely the most useful one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If exporter specs are updated to include service.namespace in their reported service, which maybe they should, then adding a special envvar for service.namespace might make sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "service name" as defined right now is unique within a namespace, and the fact that Jaeger/Zipkin don't have this concept made us use just "service name" which is wrong.
@Oberon00 I agree that exporters that only have a "service name" concept should use actually "{service.namespace}/{service.name}" as the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-telemetry/opentelemetry-specification/blob/d001d5d25ea11a28bd4c4ec2652e2db1a41fc064/specification/trace/sdk_exporters/zipkin.md#:~:text=note%2C%20the%20attribute%20service.namespace%20must%20not%20be%20used%20for%20the%20zipkin%20service%20name%20and%20should%20be%20sent%20as%20a%20zipkin%20tag.
https://github.com/open-telemetry/opentelemetry-specification/blob/b46bcab5fb709381f1fd52096a19541370c7d1b3/specification/resource/semantic_conventions/README.md#:~:text=Note,same%20string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change that, but it was intentionally not suppose to be concatenated to avoid the need to parse the namespace out of the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do understand that, and I've just added an open question to my PR. I think in case of one value string like Jaeger/Zipkin we have to guarantee the uniqueness somehow, otherwise we will have conflicts. For example 2 service names named "kafka" run by 2 teams/namespaces "a" and "b" will be seen as same service by these backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to change that. Given that namespace is empty in many cases now it is not such a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this discussion/related PR block this PR from merging?