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

Add OTEL_SERVICE_NAME environment variable #1677

Merged
merged 11 commits into from
May 24, 2021
Merged

Add OTEL_SERVICE_NAME environment variable #1677

merged 11 commits into from
May 24, 2021

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented May 7, 2021

Closes #709

Changes

Add OTEL_SERVICE_NAME environment variable to SDK configuration

@iNikem iNikem requested review from a team as code owners May 7, 2021 10:32
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

@SergeyKanzhelev You seemed to have a strong opinion on #709, so please take a look at this proposal.

specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
specification/sdk-environment-variables.md Outdated Show resolved Hide resolved
@@ -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. |
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add OTEL_SERVICE_VERSION or add a note that service.version can be set via OTEL_RESOURCE_ATTRIBUTES?

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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

I am ok with the suggestion, but before merging, let's clarify whether environment variable should take precedence over the value set in code.

owais added a commit to owais/opentelemetry-python that referenced this pull request May 7, 2021
owais added a commit to owais/opentelemetry-python that referenced this pull request May 7, 2021
owais added a commit to owais/opentelemetry-python that referenced this pull request May 7, 2021
owais added a commit to owais/opentelemetry-python that referenced this pull request May 10, 2021
owais added a commit to owais/opentelemetry-python that referenced this pull request May 10, 2021
codeboten pushed a commit to open-telemetry/opentelemetry-python that referenced this pull request May 10, 2021
@@ -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

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?

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Question that is not answer:

Why service name is treated differently than any other resource attribute?

owais added a commit to owais/opentelemetry-python that referenced this pull request May 11, 2021
@iNikem
Copy link
Contributor Author

iNikem commented May 11, 2021

Question that is not answer:

Why service name is treated differently than any other resource attribute?

  1. It is the only de-facto "required" resource attribute
  2. The majority of vendors already have similar variable (Add OTEL_SERVICE_NAME env variable #709 (comment)), so this is what end-users already expect
  3. This PR does not preclude any other PR which adds support for other resource attributes.

@iNikem
Copy link
Contributor Author

iNikem commented May 11, 2021

Question that is not answer:
Why service name is treated differently than any other resource attribute?

  1. It is the only de-facto "required" resource attribute
  2. The majority of vendors already have similar variable (#709 (comment)), so this is what end-users already expect
  3. This PR does not preclude any other PR which adds support for other resource attributes.

Based on #1677 (comment) I will submit the PR about OTEL_SERVICE_VERSION after this one is merged.

@iNikem
Copy link
Contributor Author

iNikem commented May 14, 2021

@bogdandrutu as discussed during spec SIG meeting, I tried to stress the significance of this specific attribute. Please check my changes in README.md

@@ -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 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.

@iNikem iNikem requested a review from bogdandrutu May 17, 2021 08:01
@carlosalberto
Copy link
Contributor

Ping @bogdandrutu

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

@bogdandrutu ;)

@bogdandrutu bogdandrutu dismissed their stale review May 20, 2021 22:53

Personally still believe we should not make this special but I would let the majority decide, I would not block but disagree with the change

@iNikem
Copy link
Contributor Author

iNikem commented May 23, 2021

@open-telemetry/technical-committee Anybody feeling confident enough to merge this?

@carlosalberto
Copy link
Contributor

@bogdandrutu Thanks for going on with the compromise, really appreciated. Merging (we will see in the long run how things unfold! ;) )

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.

Add OTEL_SERVICE_NAME env variable