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

obtain a tracer/meter without a name #586

Closed
zeitlinger opened this issue May 6, 2020 · 31 comments
Closed

obtain a tracer/meter without a name #586

zeitlinger opened this issue May 6, 2020 · 31 comments
Labels
area:api Cross language API specification issue area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory triage:rejected:declined

Comments

@zeitlinger
Copy link
Member

Summary

Background

In Obtaining a tracer and Obtaining a Meter, it says that name is required.

Proposed change

The name should be optional.

Use case

Obtaining a tracer/meter for the main application, rather than a library.

What should "name" be, if not provided?

Something like service.name.

Rationale

  1. Make the user API easier for the main use case (not a library).
  2. Have a consistent naming scheme for application tracer/meter, e.g. service.name.

Analysis

Problem

Provided that the resource should already be sufficient to identify an application, it seems redundant for the user to provide this information again.

In the best case, the application developer will provide the correct name (e.g. the value of service.name) - where "correct" is defined either by semantic convention, or a company internal standard.

In the worst case, the application developer will fill in some random value, which will make it harder to identify the trace/meter as being the one from the main application, which in turn could make further processing (e.g. in the collector) harder.

Possible Solutions

Static name

The name should be static, e.g. main.

Rationale: The tracer/meter can still be identified by the resource, e.g. service.name.

Pro:

  1. It's very easy to identify the tracer/meter of the main application.

Con:

  1. You always need the resource for context. However, this is already the case for library, e.g. you don't know which application created a span from the tracer io.opentelemetry.contrib.mongodb without the resource.

Service.name

The name should be filled with service.name.

Rationale: service.name is the same as (or the closest to) the application name.

Pro/Con: Inverse of the above.

Span exporter

Let the span exporter decide how to fill the tracer/meter name.

Rationale: The setup of the span exporter can be extracted into a library and be reused. In this setup logic, you can e.g. read the application name from an environment variable.

@dyladan
Copy link
Member

dyladan commented May 6, 2020

My main worry about this is if you make the name optional, library developers will leave it out. If it is required it can be statically type checked to ensure it is there.

@Oberon00
Copy link
Member

Oberon00 commented May 6, 2020

The service.name is not necessarily the same as the name of what you should put in the instrumenting library argument for the tracer. For example, your application will maybe have a separate JAR file that implements accessing some DB. Then you can use e.g. shoppingcart-dao and the version of that jar file.

Also, I think your assumption that the main use case is not a library is not necessarily correct. I would assume that most apps are sufficiently instrumented when you trace incoming HTTP, outgoing HTTP and DB-calls, which are all done in libraries that can be instrumented by potentially opentelemetry-provided instrumentation libraries. Only in special cases would you need custom spans.

@zeitlinger
Copy link
Member Author

The service.name is not necessarily the same as the name of what you should put in the instrumenting library argument for the tracer. For example, your application will maybe have a separate JAR file that implements accessing some DB. Then you can use e.g. shoppingcart-dao and the version of that jar file.

it sounds like a library, because shoppingcart-dao would be part of multiple applications.

Also, I think your assumption that the main use case is not a library is not necessarily correct. I would assume that most apps are sufficiently instrumented when you trace incoming HTTP, outgoing HTTP and DB-calls, which are all done in libraries that can be instrumented by potentially opentelemetry-provided instrumentation libraries. Only in special cases would you need custom spans.

Hard to tell. In OpenTracing, we (Zalando) currently recommend users to create custom spans (to have more insight).

@Oberon00
Copy link
Member

Oberon00 commented May 6, 2020

it sounds like a library,

@zeitlinger Yes, I think in many cases, applications will be assembled mostly from (custom) libraries.

But I wonder with your solution of using "main" as instrumenting library name, would it not be just as simple to write MAIN and MAIN is a constant defined in the application?

@zeitlinger
Copy link
Member Author

But I wonder with your solution of using "main" as instrumenting library name, would it not be just as simple to write MAIN and MAIN is a constant defined in the application?

yes, that would be possible, but defeat the purpose of this ticket - which is to remove one piece the user has to know about (and possibly get wrong).

@seh
Copy link

seh commented May 7, 2020

Per earlier discussion, in the Go tracing library, the tracer name never makes it out in the published spans.

Somewhat related: open-telemetry/opentelemetry-go#609.

@jkwatson
Copy link
Contributor

jkwatson commented May 7, 2020

The Instrumentation Library structure should be available to exporters to use in any way that they desire, though, correct?

@Oberon00
Copy link
Member

Oberon00 commented May 7, 2020

To exporters, samplers, and 3rd party SDK implementations.

@Oberon00
Copy link
Member

Oberon00 commented May 7, 2020

@zeitlinger

yes, that would be possible, but defeat the purpose of this ticket - which is to remove one piece the user has to know about (and possibly get wrong).

If you want that the user does not have to know about the instrumentation library name, you have to remove the whole instrumentation library reporting feature. By making the name optional, you may make it easier for some app developers to get it right in some circumstances (if they also set service.name) but you at the same time you make it harder for everyone else to get it right.

@reyang reyang added area:sdk Related to the SDK spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory labels Jun 26, 2020
@carlosalberto carlosalberto added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 7, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Jul 17, 2020
@bogdandrutu bogdandrutu added the area:api Cross language API specification issue label Jul 17, 2020
@bogdandrutu
Copy link
Member

Currently "empty name" is an option correct?

@Oberon00
Copy link
Member

Oberon00 commented Aug 6, 2020

An empty name is "invalid" according to the spec:

In case an invalid name (null or empty string) is specified, a working default Tracer implementation as a fallback is returned rather than returning null or throwing an exception.

-- https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations

It is currently unclear in the spec if a no-op tracers counts as "working default Tracer" or if "default Tracer" even explicitly means the no-op tracer or just a default instance of the sdk tracer to be used in case no per-name instance is used.

@Oberon00
Copy link
Member

Oberon00 commented Aug 6, 2020

In my opinion we should close this ticket without allowing an unnamed tracer (I don't know enough about meters). Making sure that each and every tracer has an associated instrumentation library name was an explicit goal of the "named tracers" effort. See open-telemetry/oteps#16 (comment) and the mentioned "Explanation"

If no name (null or empty string) is specified, following the suggestions in "error handling proposal", a "smart default" will be applied and a default Tracer / Meter implementation is returned.

So an empty or null string was considered in the OTEP and deemed an error.

@carlosalberto
Copy link
Contributor

I personally still think we should allow a 'default' Tracer. Is it ok if we discuss this item in the Specification SIG next Tuesday?

@dyladan
Copy link
Member

dyladan commented Aug 12, 2020

Was this discussed at SIG?

Personally, I think if we allow getTracer to be called without a name, libraries will inevitably do that and we will end up with a bunch of unnamed tracers in the process.

If there was a way to allow the application developer to get a tracer without a name, but not allow libraries to do the same, then I would be OK with that.

Maybe if the SDK allowed the user to acquire an unnamed tracer, but the API did not? Libraries will depend on the API only so they will not have access to this SDK method, but end users need to depend on the SDK so they will have access to that method.

@trask
Copy link
Member

trask commented Aug 12, 2020

end users need to depend on the SDK

just want to clarify that this isn't the case for end users who are using auto-instrumentation

@dyladan
Copy link
Member

dyladan commented Aug 18, 2020

end users need to depend on the SDK

just want to clarify that this isn't the case for end users who are using auto-instrumentation

I'm not sure I understand how auto-instrumentation works without an sdk, but I presume these users probably don't care about acquiring tracers anyways

@jkwatson
Copy link
Contributor

end users need to depend on the SDK

just want to clarify that this isn't the case for end users who are using auto-instrumentation

I'm not sure I understand how auto-instrumentation works without an sdk, but I presume these users probably don't care about acquiring tracers anyways

The agent has it's own copy of the SDK internally that is used. And, these users will definitely want to get a Tracer to do custom instrumentation of their own internal spans.

@kenfinnigan
Copy link
Member

I had the same question as this issue and was pointed here.

From an application developer perspective, I find it very odd to need to pass a name that is meant to identify an instrumentation library in creating a metric or tracer for their application. Especially from the metrics side, where it's more likely to be creating non-auto instrumented metrics in an application.

I don't want to derail this thread, but has there been any consideration into OTeL needing two distinct APIs, which would share common pieces? That would enable an API to exist for instrumentation libraries that potentially had many more methods and classes available, but a much simpler API for application developers to directly use.

@zeitlinger
Copy link
Member Author

but a much simpler API for application developers to directly use.

I don't think you're derailing the discussion.
I've submitted an earlier PR that already made the API simpler to use (for application developers), so if you have anything else it would make sense to mention it (possibly another PR, depends on what it is).

@arminru
Copy link
Member

arminru commented Aug 24, 2020

Would extending the TracerProvider with an API called GetApplicationTracer() and MeterProvider with GetApplicationMeter() respectively be a suitable solution for you?
This would IMHO solve the use case for application developers building instrumentation right into their app, who do not want to provide a more fine-grained instrumentation library name.
If their application is made up of different libraries and they think they might benefit from distinguishing the different instrumentation=instrumenting libraries (in the "instrumentation built-in" case both are the same) in order to see where traces/metrics originate from, they can still use the existing API to get multiple named tracers and meters.

Given the explicit term "application" in the name of the API, we would clearly mark this as exclusive to the application itself and should mostly be able to ensure that instrumentation libraries or libraries with built-in instrumentation will continue to provide their name (and version) to the provider using the existing API. This would IMHO less likely result in this information being missing compared to making the existing API optional or adding a generic "default" tracer.

The SDK implementations could either assign some hard-coded default name for the application tracer/meter when this API is used (e.g., "<application>" with no version), or even use the information for some further special handling (in future).

@carlosalberto
Copy link
Contributor

I don't want to derail this thread, but has there been any consideration into OTeL needing two distinct APIs, which would share common pieces? That would enable an API to exist for instrumentation libraries that potentially had many more methods and classes available, but a much simpler API for application developers to directly use.

There was this idea back in OpenTracing about having two APIs: a low level one (for instrumentation developers), and a high level one (for application developers). Hopefully we can play with this idea in the (near) future ;)

One small issue, for the case of having a higher level API, would be getTracer() (name-less) having to internall7 define/specify a name anyway. Not sure "" would work fine - maybe we would need to allow the usage of null ;)

@kenfinnigan
Copy link
Member

@zeitlinger thanks for helping to make the API for application developers easier to use.

@arminru your suggestion would likely work, my concern is whether the API then becomes difficult to ascertain what is the "right way" for an application developer to use without reading reams of documentation. Maybe having "application" in the name helps, but then it could raise questions as to how getTracer("name") differs.

@carlosalberto I think having two APIs makes a lot of sense, even if it's a package split API in the same artifact. For getTracer() maybe some reasonable default from "known" information about the application is appropriate.

The key problem for me is mixing the concerns of application developers and instrumentation library developers into the same API. It appears to lead to compromises in design that really suit neither party.

@tigrannajaryan
Copy link
Member

Can we remove this from the GA scope? If we decide to go ahead with this we have 3 possible ways to implement it in a backwards compatible manner:

  1. Introduce a new API to obtain an unnamed Tracer, e.g. getDefaultTracer.
  2. Make name parameter optional for getTracer.
  3. Allow empty string as a name. (Not quite as nice, but still an option).

@seh
Copy link

seh commented Sep 8, 2020

What's "a new API"? Thinking of how this would influence the Go implementation, would this mean a new method on an existing interface, or a new free function?

The proposed option 2 above would not be backward compatible in Go, because it would require different function signature.

@tigrannajaryan
Copy link
Member

@seh good questions that show there are nuances. I think the details can be discussed later, my point is that we don't necessarily need to solve it before 1.0 GA.

@andrewhsu
Copy link
Member

From the issue triage mtg today, i'm changing the label to release:after-ga since it looks like from the comments this can be punted.

@andrewhsu andrewhsu added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 9, 2020
@tsloughter
Copy link
Member

Saw this when looking at #924 and just wanted to mention another option that a number of languages might be able to provide.

It would be an interface not requiring a name but that still sets the right name when actually running. Java I don't think could do this and not sure about Go, I'd be interested to know?

This would then solve the user issue without having to introduce any new concepts.

In Erlang we do this with a mixture of compile time and runtime code. At compile time any call to the start span macro knows what Erlang module (the individual file essentially) it is in and modules can be mapped to the library it is in and its version at runtime. The library name/version is the name/version of the tracer.

This doesn't make it possible to not have to define a name in all cases (like interpreted scripts or use in the REPL) but it does make it possible in real world code.

Can Python, Ruby, JS figure out the library the start span is called from or insert at compile time this information into calls to start span at the call site?

@zeitlinger
Copy link
Member Author

Saw this when looking at #924 and just wanted to mention another option that a number of languages might be able to provide.

Hadn't thought about this option - I like it.

It would be an interface not requiring a name but that still sets the right name when actually running. Java I don't think could do this

what about https://docs.oracle.com/javase/9/docs/api/java/lang/Class.html#getModule-- ?

This would mean the current java class would have to be passed to the factory method (similar to slf4j) - and with some kotlin magic even this is not needed.

For the other languages I don't know a similar concept.

@tsloughter
Copy link
Member

Oh yea, loggers are the place to look. I would bet most of those in any language can at the very least log the "module" that the log call is made in which can then be converted to the name of the "package" (assuming the same method used by logging to get a module isn't available to get the package) when making a span.

@zeitlinger
Copy link
Member Author

Oh yea, loggers are the place to look. I would bet most of those in any language can at the very least log the "module" that the log call is made in which can then be converted to the name of the "package" (assuming the same method used by logging to get a module isn't available to get the package) when making a span.

Logrus (Go) allows access to https://godoc.org/runtime#Frame - but that does not have a module.

@Oberon00
Copy link
Member

Oberon00 commented Oct 1, 2020

Since you bring up loggers: Most logging frameworks I know require you to pass this module name manually. So why is it a problem to do the same for the instrumentation library name for the tracer?

MariusVolkhart added a commit to MariusVolkhart/opentelemetry-java that referenced this issue Oct 30, 2020
According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies open-telemetry#1879
MariusVolkhart added a commit to MariusVolkhart/opentelemetry-java that referenced this issue Nov 1, 2020
According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies open-telemetry#1879
MariusVolkhart added a commit to MariusVolkhart/opentelemetry-java that referenced this issue Nov 7, 2020
According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies open-telemetry#1879
MariusVolkhart added a commit to MariusVolkhart/opentelemetry-java that referenced this issue Nov 13, 2020
According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies open-telemetry#1879
jkwatson pushed a commit to open-telemetry/opentelemetry-java that referenced this issue Nov 13, 2020
CHANGELOG: the SDK will no longer throw an exception with a null instrumentation name on tracers or meters, but this is still not recommended usage.

* Provide a default Tracer and Meter for invalid instrumentation names

According to the spec, https://github.com/open-telemetry/opentelemetry-specification/blob/v0.6.0/specification/trace/api.md#tracerprovider-operations both a null and empty instrumentation name are invalid and a default implementation should be provided.

However, the spec is vague on what default means. See open-telemetry/opentelemetry-specification#586 (comment)

This change interprets "default" as meaning, "it should still work, but it's not ideal". Accordingly, a real Tracer or Meter implementation from the SDK is used. It meets the requirements of exporters and such by using a valid instrumentation name. It meets the requirements of application developers by producing valid tracing data. It also warns library and application developers by logging at the WARN level that an invalid value has been given.

The rational for this interpretation is that things should always work for the application developer. If an app developer doesn't provide a name, they should be chided, but they are instrumenting their own app, and likely don't care about the lack of name. If an app developer incorporates a library that doesn't specify the instrumentation name, the instrumentation should still be made available to application devs, but they should be able to turn if off (hence a valid name) and be made aware of something not being right (hence the logging) so they can report it to the library maintainers.

Fies #1879

* Mark instrumentation version nullable in MeterProvider and TracerProvider

The spec dictates this to be optional, and the implementations treat it as nullable already.

* Remove ComponentRegistry#get(String) overload

This overload is not no longer used by our code.

* fixup! Mark instrumentation version nullable in MeterProvider and TracerProvider

* fixup! Provide a default Tracer and Meter for invalid instrumentation names

* fixup! Provide a default Tracer and Meter for invalid instrumentation names

* fixup! Remove ComponentRegistry#get(String) overload

This reverts commit db56814

* Revert "Mark instrumentation version nullable in MeterProvider and TracerProvider"

This reverts commit 7e92b39
@arminru arminru closed this as not planned Won't fix, can't repro, duplicate, stale Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:sdk Related to the SDK release:after-ga Not required before GA release, and not going to work on before GA spec:metrics Related to the specification/metrics directory spec:trace Related to the specification/trace directory triage:rejected:declined
Projects
None yet
Development

No branches or pull requests