Why does BatchSpanProcessor.onStart() not include the parentContext param?

[enosi-rl]

• This only affects the JavaScript OpenTelemetry library

Related to this issue & related PR where the other onStart() signatures were updated.

The spec says:

OnStart is called when a span is started. This method is called synchronously on the thread that started the span, therefore it should not block or throw exceptions.

Parameters:

span - a read/write span object for the started span. It SHOULD be possible to keep a reference to this span object and updates to the span SHOULD be reflected in it. For example, this is useful for creating a SpanProcessor that periodically evaluates/prints information about all active span from a background thread.

parentContext - the parent Context of the span that the SDK determined (the explicitly passed Context, the current Context or an empty Context if that was explicitly requested).

BatchSpanProcessor.onStart is currently defined as:

  // does nothing.
  onStart(_span: Span): void {}

and is the only processor without the context param in its onStart().

The background for this is that I'm implementing something along the lines of the custom batch processor shown here.

When creating a span with a context that has baggage attributes but is not the active context, then the custom processor is not passed the correct context when called in the Span constructor and the baggage is not propogated.

Curious if there is a reason for this?

[Flarna]

The BatchSpanProcessor doesn't do anything in onStart therefore it's valid to skip specifying arguments it doesn't use. Even onStart() would be valid to still fulfill the implements SpanProcessor.

On caller side in SDK the context is given, see

opentelemetry-js/packages/opentelemetry-sdk-trace-base/src/Span.ts

Line 83 in ee2342b

this._spanProcessor.onStart(this, context);

But skipping the argument has the negative side effect that extending the BatchSpanProcessor like you would like to do is no longer possible as typescript complains (JS would not).

[Flarna]

created #2757

[enosi-rl]

Thanks for the quick response.

The BatchSpanProcessor doesn't do anything in onStart therefore it's valid to skip specifying arguments it doesn't use. Even onStart() would be valid to still fulfill the implements SpanProcessor.

Sorry, I understood the language reason for it, but was curious if there was a specification or other reason that I'd missed.

Obviously #2757 answers that :-)

[Flarna]

I would say it was just an oversight and as there is no tooling to tell us about such issue.

I fear that there are more similar places as it is quite common to skip arguments you don't use (to avoid that lint complains about unused arguments). Not sure if there is a lint rule to tell us such problems.

[enosi-rl]

I fear that there are more similar places

Yes, SimpleSpanProcessor has the same issue.

[Flarna]

That is covered by my PR. For this case I know the history - the additional context argument was added in spec after the spanprocessors were already implemented a while.