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

chore: create NoopSpan instead reusing NOOP_SPAN #1899

Merged
merged 10 commits into from
Feb 10, 2021

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Feb 4, 2021

Always create a fresh NoopSpan instead of returning the same NOOP_SPAN several times.

Users may attach some data to a span or use it as a key in a map.
Avoid issues/leaks in such cases by always returning a new NoopSpan.

Always create a fresh NoopSpan instead of returning the same NOOP_SPAN several times.

Users may attach some data to a span or use it as a key in a map.
Avoid issues/leaks in such cases by always returning a new NoopSpan.
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #1899 (085c1f9) into main (2395de6) will increase coverage by 0.46%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #1899      +/-   ##
==========================================
+ Coverage   92.30%   92.77%   +0.46%     
==========================================
  Files         157      172      +15     
  Lines        5122     5964     +842     
  Branches     1089     1268     +179     
==========================================
+ Hits         4728     5533     +805     
- Misses        394      431      +37     
Impacted Files Coverage Δ
packages/opentelemetry-api/src/trace/NoopSpan.ts 100.00% <ø> (ø)
packages/opentelemetry-api/src/trace/NoopTracer.ts 95.45% <66.66%> (ø)
packages/opentelemetry-api/src/context/context.ts 100.00% <100.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 95.51% <100.00%> (ø)
packages/opentelemetry-plugin-http/src/http.ts 97.31% <100.00%> (ø)
packages/opentelemetry-tracing/src/Tracer.ts 100.00% <100.00%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 87.85% <0.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 96.82% <0.00%> (ø)
packages/opentelemetry-web/src/utils.ts 94.66% <0.00%> (ø)
...mentation-xml-http-request/src/enums/EventNames.ts 100.00% <0.00%> (ø)
... and 11 more

@Flarna Flarna added the breaking label Feb 4, 2021
@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2021

Added the breaking label because NOOP_SPAN was exported on API level.

@johnbley
Copy link
Member

johnbley commented Feb 4, 2021

Added the breaking label because NOOP_SPAN was exported on API level.

Suppose we had already shipped 1.0 when you thought of this. I assume that we would

  • Document a warning to the effect mentioned above (e.g., "be aware that this is a singleton and may be a bad choice for a key")
  • Change all our usage sites to new NoopSpan() as here
  • Mark NOOP_SPAN as @deprecated and leave it in
    I'm just treating this as a thought experiment for how we will handle this in the future; I think it's fine to break this tiny corner of the public api now.

@dyladan
Copy link
Member

dyladan commented Feb 4, 2021

Added the breaking label because NOOP_SPAN was exported on API level.

Suppose we had already shipped 1.0 when you thought of this. I assume that we would

  • Document a warning to the effect mentioned above (e.g., "be aware that this is a singleton and may be a bad choice for a key")
  • Change all our usage sites to new NoopSpan() as here
  • Mark NOOP_SPAN as @deprecated and leave it in
    I'm just treating this as a thought experiment for how we will handle this in the future; I think it's fine to break this tiny corner of the public api now.

This seems like a reasonable course of action

@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2021

The missing part is to remove it on next major. Which in turn leads to questions like how often a new major will be made and if there will be some LTS support for older majors like in e.g. node.js. But that's a bigger discussion to be done outside of this PR.

But it shows once more that issues like #1765 and #1750 are needed to be handled before GA.

@dyladan
Copy link
Member

dyladan commented Feb 4, 2021

how often a new major will be made

Current thinking of the spec sig is "very rarely." last I heard was "years"

@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2021

Besides a major change in spec there could be JS local details causing majors. e.g. move to new typescript versions resulting in incompatibility with older ones,... de-support NodeJs 8,... But sure, I don't expect we reach v16 soon :o)

@MSNev
Copy link
Contributor

MSNev commented Feb 4, 2021

Suggestion:
Rather than force the usage of new NoopSpan() everywhere (which can't be minified), change to same factory pattern like

let noopSpan = createNoopSpan();

/**
 * The NoopSpan is the default {@link Span} that is used when no Span
 * implementation is available. All operations are no-op including context
 * propagation.
 */
export function createNoopSpan(): Span {
  let context: SpanContext = INVALID_SPAN_CONTEXT;
  let noopSpan:Span = {
    // Returns a SpanContext.
    context: () => context,

    // By default does nothing
    setAttribute: (_key: string, _value: unknown) => noopSpan,

    // By default does nothing
    setAttributes: (_attributes: SpanAttributes) => noopSpan,

    // By default does nothing
    addEvent: (_name: string, _attributes?: SpanAttributes) => noopSpan,

    // By default does nothing
    setStatus: (_status: SpanStatus) => noopSpan,

    // By default does nothing
    updateName: (_name: string) => noopSpan,

    // By default does nothing
    end: (_endTime?: TimeInput) => {},

    // isRecording always returns false for noopSpan.
    isRecording: () => false,

    // By default does nothing
    recordException: (_exception: Exception, _time?: TimeInput) => {}
  };

  return noopSpan;
}

This to both explicit as it's creating a new span, but we can then hide the details within the helper and the NoopSpan class code just disappears -- further assisting minification.

And size wise this becomes (could be optimized a little more)

function createNoopSpan() {
    var context = spancontext_utils_1.INVALID_SPAN_CONTEXT;
    var noopSpan = {
        // Returns a SpanContext.
        context: function () { return context; },
        // By default does nothing
        setAttribute: function (_key, _value) { return noopSpan; },
        // By default does nothing
        setAttributes: function (_attributes) { return noopSpan; },
        // By default does nothing
        addEvent: function (_name, _attributes) { return noopSpan; },
        // By default does nothing
        setStatus: function (_status) { return noopSpan; },
        // By default does nothing
        updateName: function (_name) { return noopSpan; },
        // By default does nothing
        end: function (_endTime) { },
        // isRecording always returns false for noopSpan.
        isRecording: function () { return false; },
        // By default does nothing
        recordException: function (_exception, _time) { }
    };
    return noopSpan;
}
exports.createNoopSpan = createNoopSpan;

vs the following where all of the NoopSpan.prototype.funcname are not minified.

var NoopSpan = /** @class */ (function () {
    function NoopSpan(_spanContext) {
        if (_spanContext === void 0) { _spanContext = spancontext_utils_1.INVALID_SPAN_CONTEXT; }
        this._spanContext = _spanContext;
    }
    // Returns a SpanContext.
    NoopSpan.prototype.context = function () {
        return this._spanContext;
    };
    // By default does nothing
    NoopSpan.prototype.setAttribute = function (_key, _value) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.setAttributes = function (_attributes) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.addEvent = function (_name, _attributes) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.setStatus = function (_status) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.updateName = function (_name) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.end = function (_endTime) { };
    // isRecording always returns false for noopSpan.
    NoopSpan.prototype.isRecording = function () {
        return false;
    };
    // By default does nothing
    NoopSpan.prototype.recordException = function (_exception, _time) { };
    return NoopSpan;
}());
exports.NoopSpan = NoopSpan;

@dyladan
Copy link
Member

dyladan commented Feb 4, 2021

Suggestion:
Rather than force the usage of new NoopSpan() everywhere (which can't be minified), change to same factory pattern like

let noopSpan = createNoopSpan();

/**
 * The NoopSpan is the default {@link Span} that is used when no Span
 * implementation is available. All operations are no-op including context
 * propagation.
 */
export function createNoopSpan(): Span {
  let context: SpanContext = INVALID_SPAN_CONTEXT;
  let noopSpan:Span = {
    // Returns a SpanContext.
    context: () => context,

    // By default does nothing
    setAttribute: (_key: string, _value: unknown) => noopSpan,

    // By default does nothing
    setAttributes: (_attributes: SpanAttributes) => noopSpan,

    // By default does nothing
    addEvent: (_name: string, _attributes?: SpanAttributes) => noopSpan,

    // By default does nothing
    setStatus: (_status: SpanStatus) => noopSpan,

    // By default does nothing
    updateName: (_name: string) => noopSpan,

    // By default does nothing
    end: (_endTime?: TimeInput) => {},

    // isRecording always returns false for noopSpan.
    isRecording: () => false,

    // By default does nothing
    recordException: (_exception: Exception, _time?: TimeInput) => {}
  };

  return noopSpan;
}

This to both explicit as it's creating a new span, but we can then hide the details within the helper and the NoopSpan class code just disappears -- further assisting minification.

And size wise this becomes (could be optimized a little more)

function createNoopSpan() {
    var context = spancontext_utils_1.INVALID_SPAN_CONTEXT;
    var noopSpan = {
        // Returns a SpanContext.
        context: function () { return context; },
        // By default does nothing
        setAttribute: function (_key, _value) { return noopSpan; },
        // By default does nothing
        setAttributes: function (_attributes) { return noopSpan; },
        // By default does nothing
        addEvent: function (_name, _attributes) { return noopSpan; },
        // By default does nothing
        setStatus: function (_status) { return noopSpan; },
        // By default does nothing
        updateName: function (_name) { return noopSpan; },
        // By default does nothing
        end: function (_endTime) { },
        // isRecording always returns false for noopSpan.
        isRecording: function () { return false; },
        // By default does nothing
        recordException: function (_exception, _time) { }
    };
    return noopSpan;
}
exports.createNoopSpan = createNoopSpan;

vs the following where all of the NoopSpan.prototype.funcname are not minified.

var NoopSpan = /** @class */ (function () {
    function NoopSpan(_spanContext) {
        if (_spanContext === void 0) { _spanContext = spancontext_utils_1.INVALID_SPAN_CONTEXT; }
        this._spanContext = _spanContext;
    }
    // Returns a SpanContext.
    NoopSpan.prototype.context = function () {
        return this._spanContext;
    };
    // By default does nothing
    NoopSpan.prototype.setAttribute = function (_key, _value) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.setAttributes = function (_attributes) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.addEvent = function (_name, _attributes) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.setStatus = function (_status) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.updateName = function (_name) {
        return this;
    };
    // By default does nothing
    NoopSpan.prototype.end = function (_endTime) { };
    // isRecording always returns false for noopSpan.
    NoopSpan.prototype.isRecording = function () {
        return false;
    };
    // By default does nothing
    NoopSpan.prototype.recordException = function (_exception, _time) { };
    return NoopSpan;
}());
exports.NoopSpan = NoopSpan;

This will break instanceof checks. Not sure if anyone is using these checks at the moment but is a possibility worth considering.

@MSNev
Copy link
Contributor

MSNev commented Feb 4, 2021

This will break instanceof checks. Not sure if anyone is using these checks at the moment but is a possibility worth considering.

Yes, it would and I see that in the tests, if this is really required (which I would think the idea of having a Noop version assumes that code doesn't want or need to check) then a possible "workaround" would be to check whether the span.context() === INVALID_SPAN_CONTEXT

@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2021

Currently there is class NoRecordingSpan extends NoopSpan which would also not work anymore. I know #1900 removes this.

But I just remembered that spec tells that only a Tracer is allowed to create a span. So it seems a factory method on Tracer would be needed instead of exposing the NoopSpan construtor.

I assume this is again bad for minification but IMHO the ship to use classes,.. in OTel API has sailed. I don't think that changing NoopSpan from a class to something else but keep the rest of the API is a step forward.

Otel API is not yet GA, if proper minimizing is a major concern I recommend to start now but not on single, small classes instead rework the complete API.

@Flarna
Copy link
Member Author

Flarna commented Feb 4, 2021

Yes, it would and I see that in the tests, if this is really required (which I would think the idea of having a Noop version assumes that code doesn't want or need to check) then a possible "workaround" would be to check whether the span.context() === INVALID_SPAN_CONTEXT

There are NoopSpans with valid SpanContext because of this, see

return new NoopSpan(parentFromContext);

@dyladan
Copy link
Member

dyladan commented Feb 4, 2021

There are not too many real classes in the API. The API singletons and the noop classes I think is it. It probably wouldn't take too much effort to change if minification is a real concern. The SDK is another matter.

@MSNev
Copy link
Contributor

MSNev commented Feb 4, 2021

There are not too many real classes in the API. The API singletons and the noop classes I think is it. It probably wouldn't take too much effort to change if minification is a real concern. The SDK is another matter.

Generally, minification concerns are not a single "big" thing, but more of death by a thousand cuts, so (apart from @Flarna spec link) requiring that everyone (SDK or otherwise) use new NoopSpan() vs a factory method that can get minified to m() from factoryMethod() is problematic.

But it seems that based on the spec we need to change this anyway so that only a tracer can create a span, so choosing the name of factory method on a tracer should be consideration as the length on the name has direct affects... i.e. createEmptyNoopSpan() vs noopSpan() or something similar (would depend on the usage model) etc.

But to be spec compliant, it would seem that this functionality should be moved and (ideally) the NoopSpan class removed or not exported at least from the API.

As a workaround it is possible that if a particularity method is called multiple times in a block using a local variable can help -- I don't see that in this case though.

@Flarna
Copy link
Member Author

Flarna commented Feb 5, 2021

I looked once a again into this and it seems it's not that straight forward to solve this.
Actually there is already a factory method for NoopSpan: NOOP_TRACER.startSpan().

There is a place where using NOOP_TRACER.startSpan() would look really weird as it looks like we start a span which is never ended:

return setSpan(context, new NoopSpan(spanContext));

But as this is inside API it's possible to keep using new NoopSpan() there.

@Flarna
Copy link
Member Author

Flarna commented Feb 5, 2021

Update the PR to use NOOP_TRACER.startSpan().

But not sure if we should really go this path.
Looking into java it seems they reuse also a single invalid span and they expose an api to create such a span without using tracer.
see https://github.com/open-telemetry/opentelemetry-java/blob/faa8bf867a86a180cf4a4d5cdf2a04f83d82442b/api/all/src/main/java/io/opentelemetry/api/trace/Span.java#L58-L76

Remove NoopSpan from API and use NOOP_TRACER.startSpan() to create them.
Remove NoRecordingSpan and replace creation of them by NOOP_TRACER.startSpan().
@dyladan
Copy link
Member

dyladan commented Feb 5, 2021

The prohibition on creating spans without a tracer applies primarily to users. Internally we can do what we think makes sense as long as it isn't leaked to the user. IMO we shouldn't export the NoopSpan from the API at all

@MSNev
Copy link
Contributor

MSNev commented Feb 5, 2021

The prohibition on creating spans without a tracer applies primarily to users. Internally we can do what we think makes sense as long as it isn't leaked to the user. IMO we shouldn't export the NoopSpan from the API at all

I looks like we are only using NoopSpan from within the API (exception is the ot-tracing\test\Tracer.test.ts) so we could do the above and simple NOT export the NoopSpan from the Api -- thus making it private (in theory anyway) and the Api would be "compliant" from a users perspective (for NoopSpan)

@dyladan dyladan added the API label Feb 8, 2021
@dyladan
Copy link
Member

dyladan commented Feb 9, 2021

NoopSpan is also used in the http plugin it looks like at least. This should probably be tracer.startSpan('ignored', {}, suppressInstrumentation(ROOT_CONTEXT)). I'll create another PR for this.

@Flarna
Copy link
Member Author

Flarna commented Feb 9, 2021

NoopSpan is also used in the http plugin it looks like at least.

I removed all usages of NoopSpan and replaced it by NOOP_TRACER.startSpan().

@dyladan
Copy link
Member

dyladan commented Feb 9, 2021

@open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers please review PRs that affect the API. I want to merge what we have so I can move the code to the API repository this week.

@vmarchaud
Copy link
Member

This should probably be tracer.startSpan('ignored', {}, suppressInstrumentation(ROOT_CONTEXT)). I'll create another PR for this.

You closed the PR but this PR doesn't stop instrumentation right ? I think that was the goal with the non-recording span

@dyladan
Copy link
Member

dyladan commented Feb 9, 2021

You closed the PR but this PR doesn't stop instrumentation right ? I think that was the goal with the non-recording span

Yes it does https://github.com/open-telemetry/opentelemetry-js/pull/1899/files#diff-26f74d8c9d46d751c084bc80c1cb73c74c4193f526d7ca8c5b4a0abb183b0287R590

@dyladan dyladan merged commit 000a8ac into open-telemetry:main Feb 10, 2021
@dyladan dyladan deleted the rm-noopspan branch February 10, 2021 15:58
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
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.

5 participants