ioredis: add requireParent config option

[blumamir]

• This only affects the JavaScript OpenTelemetry library
• This may affect other libraries, but I would like to get opinions here first

ioredis instrumentation library is not creating a trace if the redis operation has no parent.
While this behaviour makes sense for some users, it might not be the desired behaviour for other, as for example redis commands might be sent from timers and then they will not be instrumented. I want to add a new config option requireParent similar to the one in instrumentation-http.

Currently, HTTP instrumentation library is by default not requiring parent span, and ioredis does.
I would like to make them behave the same way, so by default all operations are instrumented, and user can choose not to instrument orphaned redis commands by setting the requireParent option to true.

Since this is a breaking change for the library, I would like to discuss it first and hear your thought about it.

[vmarchaud]

I think we could also disable instrumentation when requireParent is enabled on the http, that would avoid child-spans to be created. WDYT ?

[blumamir]

I think we could also disable instrumentation when requireParent is enabled on the http, that would avoid child-spans to be created. WDYT ?

You mean remove the creation of NoRecordingSpan, and just call the original function without any instrumentation?
I like your suggestion.
Creating NoRecordingSpan also affect the ParentBasedSampler which is not obvious to the user when setting requireParent on the plugin in my opinion.

[vmarchaud]

@blumamir Yeah i think it makes more sense than adding requireParent to every instrumentations

[obecny]

I would call it requireParentSpan to be more descriptive.

With regards to removing NoRecordingSpan, Imho the safest approach is to have Noop Implementation, if you miss somewhere some "if checks" it might fail accidently or behave differently, with Noop it will never happen and the flow will be the same. Also it will always add some extra complexity to the code, tests etc.

[blumamir]

@blumamir Yeah i think it makes more sense than adding requireParent to every instrumentations

I agree. At least the default behavior and config option names should be unified across all instrumentation libraries.

If we choose to go on the NoRecordingSpan approach, we can add this logic to the Tracer so startSpan can check the config and parent existence and return NoRecordingSpan when required.

[blumamir]

I would call it requireParentSpan to be more descriptive.

I like the name requireParentSpan 👍

With regards to removing NoRecordingSpan, Imho the safest approach is to have Noop Implementation, if you miss somewhere some "if checks" it might fail accidently or behave differently, with Noop it will never happen and the flow will be the same. Also it will always add some extra complexity to the code, tests etc.

Not sure I understood. Where do you suggest to inject this Noop implementation in the instrumentation libraries?

[obecny]

With regards to removing NoRecordingSpan, Imho the safest approach is to have Noop Implementation, if you miss somewhere some "if checks" it might fail accidently or behave differently, with Noop it will never happen and the flow will be the same. Also it will always add some extra complexity to the code, tests etc.

Not sure I understood. Where do you suggest to inject this Noop implementation in the instrumentation libraries?

I was just referring to You mean remove the creation of NoRecordingSpan, and just call the original function without any instrumentation? I would rather keep NoRecordingSpan (named by me as Noop) as opposite to adding some if statement .

[dyladan]

There is no more NoRecordingSpan. It was removed and there is only Noop now. Additionally, spec says not to create them directly. use NOOP_TRACER.startSpan instead

[obecny]

There is no more NoRecordingSpan. It was removed and there is only Noop now. Additionally, spec says not to create them directly. use NOOP_TRACER.startSpan instead

We are talking about the same, whatever it will be NoRecordingSpan, NOOP_TRACER or any other Noop object I would rather use this Noop than create extra complexity with if statements everywhere.

[blumamir]

I would rather keep NoRecordingSpan (named by me as Noop) as opposite to adding some if statement .

Maybe I am missing something, but the NoRecordingSpan / NOOP_TRACER is using if statements as well here:

  private _startHttpSpan(name: string, options: SpanOptions) {
    /*
     * If a parent is required but not present, we use a `NoopSpan` to still
     * propagate context without recording it.
     */
    const requireParent =
      options.kind === SpanKind.CLIENT
        ? this._getConfig().requireParentforOutgoingSpans
        : this._getConfig().requireParentforIncomingSpans;

    let span: Span;
    const currentSpan = getSpan(context.active());

    if (requireParent === true && currentSpan === undefined) {
      // TODO: Refactor this when a solution is found in
      // https://github.com/open-telemetry/opentelemetry-specification/issues/530
      span = NOOP_TRACER.startSpan(name, options);
    } else if (requireParent === true && currentSpan?.context().isRemote) {
      span = currentSpan;
    } else {
      span = this.tracer.startSpan(name, options);
    }
    this._spanNotEnded.add(span);
    return span;
  }

Did you mean you suggest moving the logic into the startSpan function in the Tracer and setting the config value on the Tracer when initializing the instrumentation library?

[obecny]

Some code to just explain the difference what I was talking about

Current state

  private startSpan(){
    if (this.config.requireParentSpan) {
      return this._original();
    } else {
      this._instrument();
      return this._original();
   }
  }

vs Suggested state

  private onInit(){
    if (this.config.requireParentSpan) {
      this._instrument = function () {}; // noop
    } else {
      this._instrument = function () {
        // instrument etc.
      }; 
    }
  }
  private startSpan(){
    this._instrument();
    return this._original();
  }

In second example (suggested) you assign appropriate function once only. Then next time function startSpan is being run you don't need to check for the same things again and again. Making this more complex, more if statements / else etc. and harder to test. Otherwise you just always call this._instrument() as it was already assigned during startup a proper implementation either noop or either a real instrumentation logic.

[blumamir]

@obecny , we need to also check if current span is undefined:

  private startSpan(){
    if (this.config.requireParentSpan && getSpan(context.active()) === undefined) {
      return this._original();
    } else {
      this._instrument();
      return this._original();
   }
  }

and we have to check it on each call to startSpan.

In that case, the second example becomes:

  private onInit(){
    if (this.config.requireParentSpan) {
      this._instrument = function () {
        if(getSpan(context.active()) !== undefined) {
           // call the regular instrument as usual.
        } else {
           // noop
        }
      };
    } else {
      this._instrument = function () {
        // instrument etc.
      }; 
    }
  }
  private startSpan(){
    this._instrument();
    return this._original();
  }

or something similar, right?

[obecny]

or something similar, right?

yes but you got the idea right.

[blumamir]

or something similar, right?

yes but you got the idea right.

Yes. This still requires adding if statements inside each plugin thought, but it's only one if per plugin.
What do you think about placing this code only once inside the startSpan function of the Tracer?

[dyladan]

requireParent option to startSpan?

sounds like a potential spec improvement

[obecny]

we would have to specify this in spec. Each plugin behaves differently some adds parent and few other spans, some adds only attributes to existing span. I'm not sure if this will be an easy task to define all possible edge cases. I would add this for now only for plugins that it makes sense and maybe create some shareable helper function for it - can be inside instrumentation perhaps ?

[blumamir]

requireParent option to startSpan?

sounds like a potential spec improvement

I meant adding requireParent parameter to TracerConfig which is set by InstrumentationAbstract constructor based on InstrumentationConfig when creating the Tracer for the instrumentation in getTracer(...). Then all plugins get this behavior automatically without any code change.
But now that I think about it again, it might be too general and different plugins might need specific control in specific cases.

[blumamir]

we would have to specify this in spec. Each plugin behaves differently some adds parent and few other spans, some adds only attributes to existing span. I'm not sure if this will be an easy task to define all possible edge cases. I would add this for now only for plugins that it makes sense and maybe create some shareable helper function for it - can be inside instrumentation perhaps ?

You are right, there are too many edge cases.
I think what @dyladan suggested - adding requireParent option to startSpan can keep maximum flexibility for plugins with minimum verbosity in plugins code. No need to add any if statement in plugins - there is only a single if statement inside startSpan which all plugins share.
WDYT?

[obecny]

imho good idea, we just need to make sure to make it work in the backward compatible way so that we can add this safely and it will not change the behaviour of any plugin by default unless user specify this param explicitly. Lets wait for few more votes for yes or no to see what others think.

[dyladan]

easy to make it backwards compatible if you have requireParent default false. If this is something that will be done in many places i think it makes sense. I suspect most places that use internal span type will want this type of behavior.

[blumamir]

My motivation is actually to disabled this behavior in ioredis instrumentation which is ON by default.
@obecny - since the default is currently different for different instrumentations (OFF - http, ON - ioredis \ mongodb). Do you think we should keep it as it is, or align all plugins to behave the same way?

In my opinion, we should align it to be OFF by default unless user specifically turns it ON, but that would be a behavior change for few plugins \ instrumentations.

Anyway, I can send a PR for the change in core and contrib repos when we converge to a decision.

[Flarna]

I suspect most places that use internal span type will want this type of behavior.

I'm not that sure about this. I would assume the internal spans are mostly extensions/details/annotations to their parents.
kinds server/consume => don't require a parent on default
kinds client/producer => hard to tell; maybe even dependent if the underlying protocol is able to transport spancontext (e.g. HTTP, grpc but not databases)

[blumamir]

I have to say that I'm not completely sure why it is useful.
As an application owner, I would consider it a negative feature if my app is sending database commands and performing I/O, and the tracing solution is hiding this information from me. The whole idea is to give visibility into the application.

[Flarna]

It purely depends what you expect from your tracing solution to monitor.
Some tools just display everything and consider more spans as better.
Others try to focus on user triggered requests end2end and combine/hide as much as possible to highlight the important areas (failures, long response time for end user,...).

My main point is that there is no simple right/wrong here. Trying to sync all plugins and defining the "correct" default is most likely hard.

[blumamir]

It purely depends what you expect from your tracing solution to monitor.
Some tools just display everything and consider more spans as better.
Others try to focus on user triggered requests end2end and combine/hide as much as possible to highlight the important areas (failures, long response time for end user,...).

My main point is that there is no simple right/wrong here. Trying to sync all plugins and defining the "correct" default is most likely hard.

I agree about that.
My motivation is to disable this behavior in ioredis and mongodb instrumentations, and I can align them to a reasonable default on the way if we get to a consensus.
But I'm also fine with just keeping the current per-plugin behavior and adding this requireParentSpan to config, allowing each user to choose.

[obecny]

My motivation is actually to disabled this behavior in ioredis instrumentation which is ON by default.
@obecny - since the default is currently different for different instrumentations (OFF - http, ON - ioredis \ mongodb). Do you think we should keep it as it is, or align all plugins to behave the same way?

In my opinion, we should align it to be OFF by default unless user specifically turns it ON, but that would be a behavior change for few plugins \ instrumentations.

Anyway, I can send a PR for the change in core and contrib repos when we converge to a decision.

Having in mind all different edge cases I think that requireParentSpan should be an enum instead of boolean with 3 states
0 - Default - Unset (backward compatibility)
1 - Required
2 - Not Required

This would be an optional parameter with 0 (null/undefined) as default

This way current plugins don't need to be upgraded changed or forced to change their current behaviour. And we can make necessary changes slowly.

[blumamir]

I want to summarize the two options that were suggested and discussed:

NoopSpan

Add requireParent option to startSpan function, and return NoopSpan from it when this option is turned ON and we have no active span.

With this approach, I imagine the plugin code to look something like:

span = this.tracer.startSpan(name, { requireParent: this._config.requireParentSpan });

Very clean and easy.

No Instrumentation

The other option, as suggested by @vmarchaud, is to check in each plugin code for the config and the active span, and disable instrumentation altogether.
The plugin code will look something like this:

if(this._config.requireParentSpan && getSpan(context.active()) === undefined) {
    return original.apply(this, arguments);
}

This is more verbose and error-prone than the previous options.
The benefits are:

• no effect on sampling decisions in downstream plugins, as the root span is not created. This is relevant for example if the sampler is ParentBasedSampler.
• downstream plugins which might also have requireParent configured, will not see active span when they come to make a decision.

---

I would like to fix this issue, as currently ioredis and mongodb plugins are not instrumenting operations that are initiated by a timer, which is a problem for us.
In my opinion, both options are valid. Would love to get a consensus before investing time in a PR. If not, I'll create a PR for the first approach next week and hope it will not raise any objections later :)

[dyladan]

I prefer the no instrumentation approach. The downstream plugin issue is a real one and it should also be slightly more performant. To me it is also much more obvious what is happening

[Flarna]

I think also that no instrumentation is the better choice. Has someone investigated how other languages handle this?

---

It seems this issue has moved from requireParent for ioredis to a more generic discussion. Besides the requireParent filter there are also instrumentations which have other filters, e.g. http has a url filter.
These filters are also not consistent:

• http client uses no instrumentation
• http server uses context.with(suppressInstrumentation()) which is somewhat similar to NoopSpan but additionally disables propagators

[blumamir]

Thanks @Flarna for bringing it up.
I took the time to summarize the current behavior of plugins \ instrumentation, and it looks like there is a big variation in many parameters.
Regarding requireParent, there are a few categorise currently:

non configurable requireParent == true: ioredis, mongodb, express, hapi, koa
non configurable requireParent == false: grpc-server, grpc-client, dns, mysql, pg, redis, graphql
configurable requireParent == false: http-outgoing, http-incoming

It probably makes sense to look at 3 categories for libraries as well:

• server \ consumer - http-incoming, grpc-server, graphql
• internal - express, hapi, koa
• client \ producer - ioredis, mongodb, grpc-client, dns, mysql, pg, redis, http-outgoing

Each of which might have different considerations as to setting the default behavior (for example, use suppressInstrumentation, or NoRecordingSpan to remove the entire trace starting at application entrypoint).

While there might be some undocumented reasons for setting it one way or another in specific cases, I think the project and the users can benefit from creating a unified and consistent behavior, or at least allowing user to tune this option in instrumentation configuration.
In my opinion, we should have it OFF by default - meaning instrument everything unless specifically required not to. This is of course a behavior change for 5 plugins (ioredis, mongodb, express, hapi, koa), but there is a benefit for less confusion to users.

instrumentation-http

Config Options	Default	Behavior
ignoreIncomingPaths	instrument unless listed	suppressInstrumentation
ignoreOutgoingUrls	instrument unless listed	original.call (no instrumentation)
requireParentforOutgoingSpans	instrument	NoRecordingSpan
requireParentforIncomingSpans	instrument	NoRecordingSpan

instrumentation-grpc

Config Options	Default	Behavior
ignoreGrpcMethods - grpc server	instrument unless listed	original.call (no instrumentation)
ignoreGrpcMethods - grpc client	instrument unless listed	no patching (no instrumentation)
requireParent (not configurable, always false)	instrument by default	always instrument

instrumentation\plugin-ioredis

Config Options	Default	Behavior
requireParent (not configurable, always true)	no instrument by default	original.apply (no instrumentation)

instrumentation\plugin-mongodb

Config Options	Default	Behavior
requireParent (not configurable, always true)	no instrument by default	original.apply (no instrumentation)

instrumentation\plugin-dns

Config Options	Default	Behavior
ignoreHostnames	instrument unless listed	original.apply (no instrumentation)
requireParent (not configurable, always false)	instrument by default	always instrument

instrumentation\plugin-graphql

Config Options	Default	Behavior
requireParent (not configurable, always false)	instrument by default	always instrument

plugin-express

Config Options	Default	Behavior
requireParent (not configurable, always true)	no instrument by default	original.apply (no instrumentation)
ignoreLayers	instrument unless listed	original.apply (no instrumentation)
ignoreLayersType	instrument unless listed	original.apply (no instrumentation)

plugin-hapi

Config Options	Default	Behavior
requireParent (not configurable, always true)	no instrument by default	original.apply (no instrumentation)

plugin-koa

Config Options	Default	Behavior
requireParent (not configurable, always true)	no instrument by default	original.apply (no instrumentation)

plugin-mysql

Config Options	Default	Behavior
requireParent (not configurable, always false)	instrument by default	always instrument

plugin-pg

Config Options	Default	Behavior
requireParent (not configurable, always false)	instrument by default	always instrument

plugin-redis

Config Options	Default	Behavior
requireParent (not configurable, always false)	instrument by default	always instrument