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 env variable #709

Closed
iNikem opened this issue Jul 17, 2020 · 22 comments · Fixed by #1677
Closed

Add OTEL_SERVICE_NAME env variable #709

iNikem opened this issue Jul 17, 2020 · 22 comments · Fixed by #1677
Labels
area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:miscellaneous For issues that don't match any other spec label spec:trace Related to the specification/trace directory

Comments

@iNikem
Copy link
Contributor

iNikem commented Jul 17, 2020

What are you trying to achieve?

To simplify the configuration of auto-instrumentations via environment variables, especially for first time users, add new variable OTEL_SERVICE_NAME as a way to configure service name for all exporters. As was pointed in #666 (comment), configuring service name via resources may be confusing for new users.

Additional context.
This issue assumes that #666 is merged and we have environment variables recommendation in spec.

@iNikem iNikem added the spec:miscellaneous For issues that don't match any other spec label label Jul 17, 2020
@Oberon00 Oberon00 added area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Jul 17, 2020
@Oberon00
Copy link
Member

Should this also set the service.name resource attribute?

@Oberon00
Copy link
Member

I think I was confused when reading this, because there are also exporters that have their own service name concept (like Jaeger). What should this envvar configure exactly?

@iNikem
Copy link
Contributor Author

iNikem commented Jul 17, 2020

Initial idea was that this should work with jaeger/zipkin as well. Meaning, I think, that it should configure Jaeger service name as well.

@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 23, 2020
@tedsuo
Copy link
Contributor

tedsuo commented Jul 24, 2020

Yes, it's annoying to have know which exporter is being used when setting the service name. It should also set the resource, which will keep both the resource and the exporter-specific name in sync with each other.

@reyang reyang added the priority:p3 Lowest priority level label Jul 24, 2020
@pjanotti
Copy link

pjanotti commented Jul 30, 2020

Ideally, an OTel SDK should "require" the service name, however, that conflicts with not crashing the application. Having a static fallback wouldn't help much since different services will get the same default name and it won't help to track down the exact ones missing it. What seems a reasonable compromise is to have the SDKs falling back on reflection and use the name of the main module/class or perhaps the process name itself (avoiding things like PIDs and IDs).

So regarding service name, an SDK start-up should:

  • check if service.name was set on the resources;
  • if not attempt the env var;
  • if no env var attempt via "reflection" (some languages allow it others may not, in such cases use the process name stripped of IDs, falling that at least the language/runtime pre-pended to some static string)

The spec should also indicate that exporters should be changed to not include the service name (or other items covered via resources) as part of their configuration.

@pjanotti
Copy link

A way to be more forceful is in this regard is to require the SDK to log an error and not generate data if no service name was specified (not falling back to reflection), this way all examples will have it and only people not looking at examples/docs will hit the issue.

@austinlparker
Copy link
Member

+1 to moving service name out of exporters and into SDK config. That said, I've run into some problems with auto-discovery of service names before (for example in .NET OpenTracing, people creating a tracer instance through some sort of DI mechanism sometimes caused the service name to be the name of the DI container), there's just a lot of edge cases you can run into. I'd advocate for a static/well-known string (maybe concatenate the sdk language and a timestamp and 'unknown'?)

@pjanotti
Copy link

In light of these difficulties ^ with "reflection" my tendency to prefer the "bail-out" approach: log an error with a clear message and become a no-op so users quickly will learn that it is required - and all code examples will have it. I think that a default static value is not helpful and can pollute anything based on service names.

@austinlparker
Copy link
Member

i tend to agree about not making it a static value (which is why i think if we did it, there should be some randomness, maybe even a hash?) but i think people should get into the habit of specifying it.

the only real weirdness i can think of would be people that have a single service that they wish to create multiple logical services inside of.

@iNikem
Copy link
Contributor Author

iNikem commented Jul 30, 2020

I am sorry, but this discussion is out of scope of this issue. This issue is about providing single variable to configure one specific Resource, because it is the most common requirement. How SDK/auto-instrumentation should react if such configuration is absent is totally orthogonal to this.

Spec issues and PR tend to blur to much and try to cover too much area. And then got stuck in the mud. Let's protect ourselves from scope creep.

If you feel that the question of mandatory service name is important (and I agree it its), please raise a separate issue for that. Thank you :)

@pjanotti
Copy link

Agreed @iNikem - I will open a separate issue and follow up a discussion there.

@tigrannajaryan
Copy link
Member

I think this is a nice-to-have, not a must-have for 1.0 GA. It can be added after GA in a non-breaking manner. I suggest to remove release:required-for-ga label.

@tigrannajaryan
Copy link
Member

Since I don't see any objections I am moving this to after GA.

@tigrannajaryan tigrannajaryan added release:after-ga Not required before GA release, and not going to work on before GA and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 15, 2020
@SergeyKanzhelev
Copy link
Member

Is this can be done already via #811 ?

@iNikem
Copy link
Contributor Author

iNikem commented Sep 16, 2020

Is this can be done already via #811 ?

It can, but this proposal is more intuitive for new users.

@SergeyKanzhelev
Copy link
Member

So the variable would look like this:

OTEL_RESOURCE_ATTRIBUTES="service.name=myservice"

The good thing here you can also do:

OTEL_RESOURCE_ATTRIBUTES="service.name=myservice, service.version=2.0.0"

Both looks quite intuitive.

Having two ways to configure the same setting may be more confusing. But I'm happy to hear more opinions here. If there is no specific feedback from users, should we recommend using OTEL_RESOURCE_ATTRIBUTES and close the issue? We can always re-open when there will be user feedback or we will see a lot of confusion.

@iNikem
Copy link
Contributor Author

iNikem commented Sep 16, 2020

I think my concern is discoverability. If an user looks for how to specify service name, which is a required configuration, it will be easier for them to find OTEL_SERVICE_NAME than OTEL_RESOURCE_ATTRIBUTES.

@SergeyKanzhelev
Copy link
Member

I think my concern is discoverability. If an user looks for how to specify service name, which is a required configuration, it will be easier for them to find OTEL_SERVICE_NAME than OTEL_RESOURCE_ATTRIBUTES.

The question here would be - where the user is looking for this information. If in specs repo - it is likely would be easy to point to that env variable. If in java auto-instr repo, either setting can be done there. My suggestion was to wait for real user or user-study feedback.

This actually opens up an interesting topic about docs discoverability. Especially if the concepts that vendor translate this setting to does not match the OTel concept. But it is likely a topic for another discussion.

I'm suggesting to close the issue, but totally fine keeping it for after GA.

@iNikem
Copy link
Contributor Author

iNikem commented Sep 17, 2020

Please let it live for "After GA" :)

@chgl
Copy link

chgl commented Oct 8, 2020

Not sure if you're already looking for user feedback, but I'd prefer OTEL_SERVICE_NAME over resource attributes. Setting a single value is much easier than maintaining a list of key-value pairs, especially when the name may change per environment. E.g. one may use Kubernetes' fieldRef to set the value of the OTEL_SERVICE_NAME to the name of a pod or a label. Or load it from a config file. Or auto-inject it into the container based on a set of labels/namespace information as the Jaeger Operator does.

@arminru
Copy link
Member

arminru commented Oct 9, 2020

@chgl We are always looking for user feedback - thanks a lot for you input!

@owais
Copy link
Contributor

owais commented Apr 5, 2021

For most APM products, service is probably the most important configuration option. This is highlighted by the fact that almost all Otel distributions specify their own config option for service name. For example:

https://github.com/lightstep/otel-launcher-java#system-properties-and-environmental-variables
https://github.com/signalfx/splunk-otel-python#all-configuration-options
https://github.com/DataDog/dd-opentelemetry-exporter-js#configuration-options---tagging
https://github.com/newrelic/opentelemetry-exporter-java#configuration-system-properties
https://github.com/honeycombio/honeycomb-opentelemetry-java#configuration

This show a clear need for a dedicated config option to specify the service.name resource attribute. It'd be really nice if Otel supported this out of the box.

iNikem added a commit that referenced this issue May 7, 2021
carlosalberto pushed a commit that referenced this issue May 24, 2021
* Add OTEL_SERVICE_NAME environment variable

Closes #709
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
* Add OTEL_SERVICE_NAME environment variable

Closes open-telemetry#709
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 release:after-ga Not required before GA release, and not going to work on before GA spec:miscellaneous For issues that don't match any other spec label spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.