chore: create NoopSpan instead reusing NOOP_SPAN #1899

Flarna · 2021-02-04T11:25:29Z

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 · 2021-02-04T11:31:59Z

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 · 2021-02-04T12:48:03Z

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

johnbley · 2021-02-04T16:21:16Z

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 · 2021-02-04T16:23:45Z

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 · 2021-02-04T17:39:58Z

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 · 2021-02-04T17:55:36Z

how often a new major will be made

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

Flarna · 2021-02-04T18:08:50Z

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 · 2021-02-04T20:22:38Z

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 · 2021-02-04T21:03:00Z

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 · 2021-02-04T21:14:00Z

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 · 2021-02-04T21:19:00Z

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 · 2021-02-04T21:20:35Z

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

opentelemetry-js/packages/opentelemetry-api/src/trace/NoopTracer.ts

Line 40 in 3b5892b

return new NoopSpan(parentFromContext);

dyladan · 2021-02-04T21:27:26Z

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 · 2021-02-04T22:00:22Z

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 · 2021-02-05T11:45:53Z

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:

opentelemetry-js/packages/opentelemetry-api/src/context/context.ts

Line 68 in 7e423a9

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

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

Flarna · 2021-02-05T12:16:28Z

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 · 2021-02-05T15:56:36Z

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 · 2021-02-05T20:05:54Z

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 · 2021-02-09T16:49:49Z

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 · 2021-02-09T16:56:00Z

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 · 2021-02-09T17:06:58Z

@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 · 2021-02-09T19:27:05Z

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 · 2021-02-09T19:58:26Z

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

chore: create NoopSpan instead reusing NOOP_SPAN

3ffd996

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.

Flarna requested review from dyladan, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners February 4, 2021 11:25

Flarna added the breaking label Feb 4, 2021

johnbley approved these changes Feb 4, 2021

View reviewed changes

Merge branch 'main' into rm-noopspan

1695c32

Merge remote-tracking branch 'upstream/main' into rm-noopspan

05a1ae2

chore: use NOOP_TRACER.startSpan()

2b46339

Remove NoopSpan from API and use NOOP_TRACER.startSpan() to create them. Remove NoRecordingSpan and replace creation of them by NOOP_TRACER.startSpan().

Merge branch 'main' into rm-noopspan

092fdb3

Merge branch 'main' into rm-noopspan

0d7c8d8

dyladan added the API label Feb 8, 2021

Flarna added 2 commits February 8, 2021 17:55

Merge remote-tracking branch 'upstream/main' into rm-noopspan

42d6f0c

chore: fix merge errors

877b780

Merge branch 'main' into rm-noopspan

2ad54f0

dyladan mentioned this pull request Feb 9, 2021

chore: replace noop span creation with context suppression #1919

Closed

dyladan approved these changes Feb 9, 2021

View reviewed changes

Merge branch 'main' into rm-noopspan

fe492e1

vmarchaud approved these changes Feb 9, 2021

View reviewed changes

dyladan merged commit 000a8ac into open-telemetry:main Feb 10, 2021

dyladan deleted the rm-noopspan branch February 10, 2021 15:58

Flarna mentioned this pull request Apr 27, 2021

[spec] Do not export NOOP implementations open-telemetry/opentelemetry-js-api#50

Closed

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

chore: create NoopSpan instead reusing NOOP_SPAN (open-telemetry#1899)

453a101

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

chore: create NoopSpan instead reusing NOOP_SPAN (open-telemetry#1899)

fb674d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: create NoopSpan instead reusing NOOP_SPAN #1899

chore: create NoopSpan instead reusing NOOP_SPAN #1899

Flarna commented Feb 4, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

Flarna commented Feb 4, 2021

johnbley commented Feb 4, 2021

dyladan commented Feb 4, 2021

Flarna commented Feb 4, 2021

dyladan commented Feb 4, 2021

Flarna commented Feb 4, 2021

MSNev commented Feb 4, 2021

dyladan commented Feb 4, 2021

MSNev commented Feb 4, 2021

Flarna commented Feb 4, 2021

Flarna commented Feb 4, 2021 •

edited

Loading

dyladan commented Feb 4, 2021

MSNev commented Feb 4, 2021

Flarna commented Feb 5, 2021

Flarna commented Feb 5, 2021 •

edited

Loading

dyladan commented Feb 5, 2021

MSNev commented Feb 5, 2021

dyladan commented Feb 9, 2021

Flarna commented Feb 9, 2021

dyladan commented Feb 9, 2021

vmarchaud commented Feb 9, 2021

dyladan commented Feb 9, 2021

chore: create NoopSpan instead reusing NOOP_SPAN #1899

chore: create NoopSpan instead reusing NOOP_SPAN #1899

Conversation

Flarna commented Feb 4, 2021

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

Flarna commented Feb 4, 2021

johnbley commented Feb 4, 2021

dyladan commented Feb 4, 2021

Flarna commented Feb 4, 2021

dyladan commented Feb 4, 2021

Flarna commented Feb 4, 2021

MSNev commented Feb 4, 2021

dyladan commented Feb 4, 2021

MSNev commented Feb 4, 2021

Flarna commented Feb 4, 2021

Flarna commented Feb 4, 2021 • edited Loading

dyladan commented Feb 4, 2021

MSNev commented Feb 4, 2021

Flarna commented Feb 5, 2021

Flarna commented Feb 5, 2021 • edited Loading

dyladan commented Feb 5, 2021

MSNev commented Feb 5, 2021

dyladan commented Feb 9, 2021

Flarna commented Feb 9, 2021

dyladan commented Feb 9, 2021

vmarchaud commented Feb 9, 2021

dyladan commented Feb 9, 2021

codecov bot commented Feb 4, 2021 •

edited

Loading

Flarna commented Feb 4, 2021 •

edited

Loading

Flarna commented Feb 5, 2021 •

edited

Loading