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

When should an instrumentation library set a service name attribute? #454

Closed
sagikazarmark opened this issue Nov 17, 2020 · 8 comments
Closed
Assignees
Labels
area: instrumentation Related to an instrumentation package
Milestone

Comments

@sagikazarmark
Copy link
Contributor

Looking at the current tracing implementations: some of them accept a service name and set that name in the trace, in addition to an operation name (use as the span name).

Is there a convention for accepting a service name? If there is, it's not immediately obvious, because I can see a service name appearing in both server (eg. http) and client (eg. memcached) components as well.

I would assume that setting a service name is enough in the first span of a service, but I could be wrong. So what's the convention here?

Thanks!

@matej-g
Copy link
Contributor

matej-g commented Nov 18, 2020

Could you elaborate on the question? Is this regarding adding service / operation name as a span attribute? Or am I misunderstanding.

@sagikazarmark
Copy link
Contributor Author

@matej-g thanks for your response.

otelmux accepts a service name in its constructor and automatically guesses the operation from the route.

Internally, it calls semconv.HTTPServerAttributesFromHTTPRequest that sets the service name as a span attribute.

It's not obvious to me when should an instrumentation library accept and set a service name attribute in a span. Should all libraries do that?

In retrospect, the operation name has probably nothing to do with the question.

@sagikazarmark sagikazarmark changed the title Service name and/vs operation name When should an instrumentation library set a service name attribute? Nov 18, 2020
@matej-g
Copy link
Contributor

matej-g commented Nov 18, 2020

Got it now. In general, best resource to consult on attributes requirements is the specification, in particular the sections on semantic conventions; in our case it's https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/trace/semantic_conventions.

Looking at the otelmux, it accepts string parameter service in Middleware method. However, according to the documentation and as it seems from usage in HTTPServerAttributesFromHTTPRequest, it is using this parameter to populate attribute for http.server_name (relevant part of the spec). So I would assume setting this attribute is specific to HTTP server-related spans only.

On the other hand, I see some other instrumentations (namely otelmemcache and otelmongo) are setting service.name attribute, which seems to go above what the spec requires (trace convention does not mention service.name, but this attribute exists in resources convention). It makes me think this attribute is not needed in these two modules.

From my reading, the bottom line is: Unless you're dealing with HTTP server spans, you don't need to set service (or in this case HTTP server) name attribute.

@sagikazarmark
Copy link
Contributor Author

Thanks! That's kind of what I figured, but otelmemcache was a bit confusing.

@matej-g
Copy link
Contributor

matej-g commented Nov 18, 2020

Yes, I think it should not be setting that attribute, seems redundant (shamefully admitting, I authored that module). Thanks for bringing this up!

@Aneurysm9
Copy link
Member

On the other hand, I see some other instrumentations (namely otelmemcache and otelmongo) are setting service.name attribute, which seems to go above what the spec requires (trace convention does not mention service.name, but this attribute exists in resources convention). It makes me think this attribute is not needed in these two modules.

I think this is correct and those modules should not be setting that attribute. service.name is a resource attribute that should be set by the application utilizing the instrumentation and not by an instrumentation itself.

@evantorrie
Copy link
Contributor

I think this is correct and those modules should not be setting that attribute. service.name is a resource attribute that should be set by the application utilizing the instrumentation and not by an instrumentation itself.

Correct.

service.name should always be derived from the Resource associated with the Span. In the Go implementation this Resource is set via the client code passing a WithResource(resource) option to NewTracerProvider(), i.e.

provider := NewTracerProvider(WithResource(resource))

If the code does not pass a WithResource() option, the TracerProvider is expected to use resource.Default().

While one could create a service.name attribute and associate it with a Span, the exporters such as OTLP, Jaeger and Zipkin will ignore this when converting a Span into its specific data model representation of the telemetry-producing entity.

As such, we need to survey existing instrumentations in this repo and remove cases where they implicitly set a service.name attribute.

I will create PRs where needed.

@evantorrie
Copy link
Contributor

With the merging of #763 I believe there are no longer opentelemetry-go-contrib instrumentations which allow specifying a service.name other than via associating a Resource with a TracerProvider - and eventually also via a MeterProvider when the metrics API resettles.

I believe that means we can close this issue. Will move to review in the OpenTelemetry Release Candidate project.

@MrAlias MrAlias closed this as completed May 6, 2021
plantfansam referenced this issue in plantfansam/opentelemetry-go-contrib Mar 18, 2022
* Drop entry from correlation map

Entry used to contain stuff like TTL, but right now the notion of
entry was dropped from the spec.

* Compute exact size of the correlations map

The map will be immutable, so spend some more time to ensure that we
will not unnecessarily waste some memory on a basically read-only map.

* Allow dropping keys from correlations map

This is to follow the spec. Before this change it was possible in an
awkward way by using Foreach to gather and filter the key-value pairs,
and then calling NewMap with a MultiKV MapUpdate.

* Document the correlation package

* Add missing license blurbs in correlation package

* Add tests for deleting items in correlations

* Factor out getting map size and test it

This is an implementation detail that can't be tested in a black-box
manner.

* Fix copyright dates

* Simplify/disambiguate keySet function parameters/return values

* Fix typo in Apply docs

* Fix test names

* Explain the nonsense of dropping keys from new map
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package
Projects
None yet
Development

No branches or pull requests

5 participants