feat(instrumentation): add util to execute span customization hook in base class

[blumamir]

TL;DR

this PR will allow us to clean up dozens of places in contrib that implemented this code or similar:

        if (self._config.consumeHook) {
          safeExecuteInTheMiddle(
            () => self._config.consumeHook!(span, { moduleVersion, msg }),
            e => {
              if (e) {
                diag.error('amqplib instrumentation: consumerHook error', e);
              }
            },
            true
          );
        }

And transform it into a shorter, consistent and intuitive:

this.runInstrumentationEventHook(self._config.consumeHook, 'consumeHook', span, { moduleVersion, msg })

Motivation

Over time and due to user requests, instrumentations in core and contrib evolved to offer a popular extension point to allow recording of additional data on spans.

The mechanism is implemented via a user hook that can be registered in the instrumentation config for each individual instrumentation, for example: amqplib:

export interface AmqplibInstrumentationConfig extends InstrumentationConfig {
  /** hook for adding custom attributes before publish message is sent */
  publishHook?: AmqplibPublishCustomAttributeFunction;

  /** hook for adding custom attributes after publish message is confirmed by the broker */
  publishConfirmHook?: AmqplibPublishConfirmCustomAttributeFunction;

  /** hook for adding custom attributes before consumer message is processed */
  consumeHook?: AmqplibConsumeCustomAttributeFunction;

  /** hook for adding custom attributes after consumer message is acked to server */
  consumeEndHook?: AmqplibConsumeEndCustomAttributeFunction;

Then the user can choose to initialize the instrumentation with custom hooks that can record, for example, one can collect the payload like this:

const amqplibPublishHook = (span, publishInfo) => {
    if (publishInfo.content !== undefined) {
        span.setAttribute("my.custom.attribute.name", publishInfo.content.toString());
    }
};

When instrumentation invoke the hook, it commonly looks like this:

        if (self._config.consumeHook) {
          safeExecuteInTheMiddle(
            () => self._config.consumeHook!(span, { moduleVersion, msg }),
            e => {
              if (e) {
                this._diag.error('consumerHook error', e);
              }
            },
            true
          );
        }

There are multiple variations which I think can be hoisted to the base class, to a function that executes the hook if it is present, and catch any exceptions from the user-supplied function so that we don't crash anything. If there is an error, it logs it to the instrumentation diag channel and silently returns.

I want to suggest adding the following function to InstrumentationAbstract as a new feature:

  protected runInstrumentationEventHook<EventInfoType>(
    hookHandler: InstrumentationEventHook<EventInfoType> | undefined,
    eventName: string,
    span: Span,
    eventInfo: EventInfoType
  ) {
    if (!hookHandler) {
      return;
    }

    try {
      hookHandler(span, eventInfo);
    } catch (e) {
      this._diag.error(`Error running span hook for event ${eventName}`, e);
    }
  }

And after it is merged, align all instrumentations to use it. The benefits are:

• less duplicate code lines across different packages in this repo to maintain
• consistent handling of the hook flow and diag message
• correct typing for the hooks, to promote consistency and type safety
• abstracting away common task from dozens of instrumentations into base class for reuse

Migration Plan

There is currently inconsistency in the existing instrumentations hook signatures. This PR requires all instrumentations to align according to a common standard, which is:

(span: Span, eventInfo: EventInfoType) => void;

Where as some instrumentations in contrib are using different signatures like redis:

export interface RedisResponseCustomAttributeFunction {
  (
    span: Span,
    cmdName: RedisCommand['command'],
    cmdArgs: RedisCommand['args'],
    response: unknown
  ): void;
}

Since this PR adds a feature, consumers can migrate to this util at any time.

For the packages where the hook signature is already correct, this is an easy task that can be applied right after core is released. these are:

• amqplib
• mongoose
• socket.io
• aws-lambda
• aws-sdk
• cassandra
• express
• fastify
• koa
• mongodb
• mysql2
• pg
• restify

The hooks that are structured other than the common way would need to align as a breaking change, either now or in the future. these are:

• fetch
• http
• xhr
• fs
• unidici
• graphql
• ioredis
• redis-4
• redis

[blumamir]

I chose to make the generic type EventInfoType required, to promote typings of the object.

Since some instrumentations are not able to type the object correctly, which might require type export from the instrumented package itself, it is not always possible. In this case the instrumentation can choose to use any in the hook, which communicates that the type was ignored on purpose

[blumamir]

@open-telemetry/javascript-approvers This PR also introduces a new name: "InstrumentationEvent". At the moment, instrumentation has partially settled on a hook function signature and behaviour. The pattern is a "{some event name}Hook" function that receive a span along with info object, at specific times in the lifecycle of a span it records, and the hook is used to add additional attributes.

I wanted to document it as a type and give it a name that we can use which will promote consistency and reduce mental load for instrumentation writers and maintainers.

WDYT about the name "InstrumentationEvent"? It can only be used in the context of a specific open span (for which we can add attributes), required info object, and no return value.

[blumamir]

This function uses this._diag to silently log any errors from hooks. thus the function needs to have access to this and is defined as a class method.

It is marked protected so that we can only run it from a class method but it is not exposed on the public API, similarly to the existing _wrap, _unwrap etc functions.

[blumamir]

I was wondering if it can be useful to also always include the eventName as an attribute in the event info, something like:

Suggested change

	hookHandler(span, eventInfo);
	hookHandler(span, {eventName, ...eventInfo});

and adjust the types.

The hook can always choose to ignore this parameter but it will be available if needed. Not sure thought if it's worth adding more complications to the code for a non existing use that I'm aware of at the moment.

[Flarna]

This would be helpful if a hook function is used for several hooks. I have no usecase in mind. Price is the extra object created.

[blumamir]

My thinking as well. thanks. will wait for more opinions.

We can also add it in the future in a non breaking manner

[blumamir]

As part of this PR, I also transformed the diag message to be more structured - make the log message constant and add the eventName as an argument so it is easy to consume.

I believe these type of diag prints are better and should be preferred, but am open to address this in a different PR

[Flarna]

Should we consider to generalize the hooks even further to allow more users?
E.g. like an EventEmitter (requires to get the instance to register a hook) or even use Diagnostics Channel?

[blumamir]

Thanks @Flarna

Intresting.

I think that we can explore other mechanisms for instrumentations to register custom span hooks to enhance attributes or span name, which will make the publishing and consumption more fluent.

Since each hook carries a unique info object, I love how easy it is now to become aware of all the hooks with their types right in the configuration object for each instrumentation. Since the hook needs to be synchronous and set any additional attributes before the span ends, the current syntax make it more obvious, where publish-subscribe patterns are sometimes harder to trace in code.

A notable benefit of event emitter IMO is the opportunity for otel distributions to reuse hooks code and register hooks in a consistent and structural manner.

Should we consider to generalize the hooks even further to allow more users? E.g. like an EventEmitter (requires to get the instance to register a hook)

My understanding about EventEmitters is that they are less intuitive to work with and type, and it seems they haven't been introduced anywhere in the project yet. Or are you suggesting using something like EventEmitter that we write?

Another change for using an event emitter is that the hooks registration will not happen in the constructor on object creation, but will now be separated to a different call after an instance has been created.

or even use Diagnostics Channel?

As the hook is used to add custom attributes to spans, and not for diagnostic purposes, I think it's not a perfect fit here semantically and can lead to confusions.

[Flarna]

Yes, my comment was to think about reusing other exiting mechanisms.

Both EventEmitter and diagnostics channel are sync and allow mutation. Typing could be added in the individual instrumentation similar as it is done in node.

But I agree that there is a major semantic difference. Hooks here are intended for data mutation and/or influence instrumentation behavior whereas above are intended to just provide data.

Besides that instrumentations are not just a node thing, they are also for browser. Your solution doesn't require polyfills.

[blumamir]

@Flarna Thank you for the review 🙏
Addressed your comments

I hope this new thing that I named is common enough to deserve it's own type and execute util function. We can consider also hoisitng utils for other usecases in followup PRs if they are common and structured IMO. I also hope that we don't end up with multiple hook classes for different cases with confusing names and scopes

[pichlermarc]

Hmm, I wonder if we can make this whole protected method actually a stand-alone function.

As a protected method it's actually part of the public interface anyway, so adding it as a stand-alone function does turn out to be the same. I'm worried that, as more signals become available, we add a lot of util functions to the instrumentation base that will make it blow up in size over time.

Adding things to abstract classes is also difficult once stable, as in typescript consumers can use the type like:

// uses instrumentation 1.1 (new protected method in InstrumentationBase)
import {InstrumentationBase} from '@opentelemetry/instrumentation';
// uses instrumentation 1.0 (missing protected method from 1.1, uses pinned for some reason 1.0 as dependency)
import {MyInstrumentation} from '@opentelemetry/my-cool-instrumentation';

// this assignment will not compile as InstrumentationBase (1.1) is defining more protected properties than InstrumentationBase (1.0) used by MyInstrumentation
const myInstrumentation: InstrumentationBase = new MyInstrumentation();

We've seen this happen with the (non-abstract) Resource class #3676

I feel putting this here now may spark lots of requests later to add more causing us to run into a similar problem (I'll be less impactful than the Resource one, as I don't assume people use it like I described above a lot, but it can still happen and can be breaking).

It's actually something that happens occasionally with MetricReader too, especially with @opentelemetry/sdk-node where @opentelemetry/sdk-metrics is pinned.

[blumamir]

I would love to have it as util, but since it's using this._diag I need access to this which felt like a class member function.

Having it as util would mean passing also the diag logger as an argument to the function, which felt strange to me.

WDYT?

[blumamir]

Adding things to abstract classes is also difficult once stable, as in typescript consumers can use the type like:

// uses instrumentation 1.1 (new protected method in InstrumentationBase)
import {InstrumentationBase} from '@opentelemetry/instrumentation';
// uses instrumentation 1.0 (missing protected method from 1.1, uses pinned for some reason 1.0 as dependency)
import {MyInstrumentation} from '@opentelemetry/my-cool-instrumentation';

// this assignment will not compile as InstrumentationBase (1.1) is defining more protected properties than InstrumentationBase (1.0) used by MyInstrumentation
const myInstrumentation: InstrumentationBase = new MyInstrumentation();

For the code snippet above, I guess it would make more sense to use the Instrumentation interface and not type something as InstrumentationBase directly, although no one will stop users from doing this.

Do you think once stable we should avoid adding new methods to InstrumentationAbstract, or are we still allowed to do that but should try not to if possible?

[pichlermarc]

I would love to have it as util, but since it's using this._diag I need access to this which felt like a class member function.

Hmm, I see, let's leave it like this then and re-evaluate later.

Do you think once stable we should avoid adding new methods to InstrumentationAbstract, or are we still allowed to do that but should try not to if possible?

Hard to say. I think we should at least try not to do it. I've tried to find resources on this topic (specifically related to typescript libraries) but I was unsuccessful in finding a concrete answer to the question (which I've had now for quite a while). Most of the resources online and in books are specifically related to TypeScript in apps which is unfortunately not as helpful for my specific questions as a library author.

From what I've seen users expect that these breakages should not occur outside of major versions and, intuitively, I agree. For the MetricReader we've done it before simply because that's the only way we could make any changes at all. Impact on end-users is fairly minimal.

For TypeScript, it is extremely easy to break users when exporting classes, this is further complicated by having situations that allow for multiple versions of the same package, and the fact that private class members are part of the public interface, too. Adding anything is a nightmare when being strict enough.

What that'd mean for us is that:

• we should not export classes as a public API
  • we can only use functions (factory functions) and interfaces, which we may extend with optional properties/parameters
• we should also not export abstract classes, but work interfaces and composition (injecting a helper through an interface method) where possible

I'm not sure if that's right or the the way to go though. Let's leave this PR as-is and discuss this another time. It's likely a larger change that will affect all components and needs to be carefully considered.

[blumamir]

Thanks for sharing your thoughts and describing the problem so clearly. I agree, it's best to leave this PR as-is and reevaluate later 🙏

What that'd mean for us is that:

• we should not export classes as a public API

  • we can only use functions (factory functions) and interfaces, which we may extend with optional properties/parameters

• we should also not export abstract classes, but work interfaces and composition (injecting a helper through an interface method) where possible

Trying to think how these considerations can be applied in the InstrumentationBase case. would love to discuss this later on

[pichlermarc]

Looks good, just two nits 🙂

[blumamir]

fixed for #4659 which is not released yet

[blumamir]

moved this message to experimental/CHANGELOG.md