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

OpenTelemetry.getTracer(...) differs from API spec #1879

Closed
MariusVolkhart opened this issue Oct 25, 2020 · 2 comments · Fixed by #1941
Closed

OpenTelemetry.getTracer(...) differs from API spec #1879

MariusVolkhart opened this issue Oct 25, 2020 · 2 comments · Fixed by #1941
Labels
Bug Something isn't working help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release

Comments

@MariusVolkhart
Copy link
Contributor

Describe the bug
The OpenTelemetry.getTracer(...) family of methods claim to reject a null library name. The spec for obtaining a tracer indicates that invalid names such as null and empty string should lead to a default value.

This behavior (returning a default) is found when only the API is on the classpath, but the SDK rejects null and throws an exception.

Steps to reproduce

OpenTelemetry.getGlobalTracer(null);
OpenTelemetrySdk.get().getTracer(null);

What did you expect to see?
I don't know what the right answer is, but it's odd to me that the API silently allows null (but documents it as invalid), while the SDK throws on null, and the spec suggests null should be silently handled.

What version and what artifacts are you using?
Artifacts: opentelemetry-api, opentelemetry-sdk
Version: v0.9.1, 0.10.0-SNAPSHOT

@MariusVolkhart MariusVolkhart added the Bug Something isn't working label Oct 25, 2020
@jkwatson jkwatson added release:required-for-ga Required for 1.0 GA release priority:p2 Medium priority issues and bugs. labels Oct 26, 2020
@dengliming
Copy link
Member

dengliming commented Oct 27, 2020

What about adding an overloaded method like:

default Tracer getTracer() {
    return DefaultTracer.getInstance();
  }

and we don't need to check the null library name.

@anuraaga
Copy link
Contributor

I like the idea of providing an API to get a tracer without a name, but FWIU, the spec is talking about it but hasn't really come to a conclusion on whether we should proceed with it or not.

open-telemetry/opentelemetry-specification#586

Either way, we would still have the methods that accept string and may be passed null, so in the meantime I think we should fix the bug where the SDK is throwing an exception on null, it should return a tracer with either an empty string name or a name like "default" if an empty string name isn't allowed, not sure off the top of head.

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 6, 2020
The parameters were already marked as being @nonnull. However, the SDK implementation failed when null was passed, while the API allowed null. The result is that one may publish a library that compiles and tests fine, but fails when the SDK is applied by an application developer.

This change adds test to verify the non-null behavior is consistently enforced, and adds checks to the API to enforce non-null. These checks will prevent library authors from creating libraries that crash at runtime.

Fixes 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working help wanted priority:p2 Medium priority issues and bugs. release:required-for-ga Required for 1.0 GA release
Projects
None yet
4 participants