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_SERVICE_NAME environment variable #1677

Merged
merged 11 commits into from
May 24, 2021
Merged

Add OTEL_SERVICE_NAME environment variable #1677

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
22cc59f
Add OTEL_SERVICE_NAME environment variable
iNikem May 7, 2021
987e435
Add changelog
iNikem May 7, 2021
1e47163
Update PR link in changelog
iNikem May 7, 2021
df4cddc
Polish text
iNikem May 7, 2021
239dc6d
Polishing text
iNikem May 7, 2021
6c9278c
Update specification/sdk-environment-variables.md
iNikem May 7, 2021
0d91482
More clearly say that service.name is a special attribute
iNikem May 14, 2021
cb734a9
Merge remote-tracking branch 'origin/iNikem-patch-1' into iNikem-patch-1
iNikem May 14, 2021
dda6729
Update specification/resource/semantic_conventions/README.md
iNikem May 18, 2021
9169d40
Merge remote-tracking branch 'upstream/main' into iNikem-patch-1
iNikem May 20, 2021
058d606
Merge branch 'main' into iNikem-patch-1
carlosalberto May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ release.

### SDK Configuration

- Add `OTEL_SERVICE_NAME` environment variable. ([#1677](https://github.com/open-telemetry/opentelemetry-specification/pull/1677))
Copy link
Member

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.

Copy link
Contributor Author

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 :)

Copy link
Member

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.

Copy link
Member

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)

Copy link
Member

@bogdandrutu bogdandrutu May 18, 2021 •
edited
Loading

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

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.

Copy link
Member

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.

Note, the attribute service.namespace MUST NOT be used for the Zipkin service name and should be sent as a Zipkin tag.

https://github.com/open-telemetry/opentelemetry-specification/blob/b46bcab5fb709381f1fd52096a19541370c7d1b3/specification/resource/semantic_conventions/README.md#:~:text=Note,same%20string

Note: service.namespace and service.name are not intended to be concatenated for the purpose of forming a single globally unique name for the service. For example the following 2 sets of attributes actually describe 2 different services (despite the fact that the concatenation would result in the same string):

Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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?


## v1.3.0 (2021-05-05)

### Context
Expand Down
1 change: 1 addition & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ Note: Support for environment variables is optional.
|Feature |Go |Java|JS |Python|Ruby|Erlang|PHP|Rust|C++|.Net|Swift|
|----------------------------------------------|---|----|---|------|----|------|---|----|---|----|-----|
|OTEL_RESOURCE_ATTRIBUTES | + | + | + | + | + | + | - | + | - | + | - |
|OTEL_SERVICE_NAME | | | | | | | | | | | |
|OTEL_LOG_LEVEL | - | - | + | [-][py1059] | + | + | - | | - | - | - | |
|OTEL_PROPAGATORS | - | + | | + | + | + | - | - | - | - | - |
|OTEL_BSP_* | - | + | | + | + | + | - | + | - | - | - |
Expand Down
1 change: 1 addition & 0 deletions specification/sdk-environment-variables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 | Value of the [`service.name`](./resource/semantic_conventions/README.md#service) resource attribute | | If both this and `OTEL_RESOURCE_ATTRIBUTES` are provided then `OTEL_SERVICE_NAME` is [merged into](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#merge) `OTEL_RESOURCE_ATTRIBUTES`
iNikem marked this conversation as resolved.
Show resolved Hide resolved
iNikem marked this conversation as resolved.
Show resolved Hide resolved
iNikem marked this conversation as resolved.
Show resolved Hide resolved
| 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) |
Expand Down