feat(instrumentation-ioredis): add request hook

[blumamir]

Which problem is this PR solving?

fixes #411

• ioredis instrumentation: support capturing the instrumented ioredis package version and set it as span attribute.

Short description of the changes

• added the request hook which user can use to capture additional data from db request, such as moduleVersion, command name, command arguments

[dyladan]

Is there semconv spec for this?

[blumamir]

Is there semconv spec for this?

no, this is why we allow the user to specify the attribute name and not using hardcoded value

[codecov]

Codecov Report

Merging #415 (ec1e3d1) into main (041508f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #415      +/-   ##
==========================================
+ Coverage   95.21%   95.23%   +0.01%     
==========================================
  Files         133      133              
  Lines        8178     8223      +45     
  Branches      804      807       +3     
==========================================
+ Hits         7787     7831      +44     
- Misses        391      392       +1     

Impacted Files	Coverage Δ
...entelemetry-instrumentation-ioredis/src/ioredis.ts	93.54% <0.00%> (ø)
...metry-instrumentation-ioredis/test/ioredis.test.ts	95.56% <0.00%> (+0.18%)	⬆️
...opentelemetry-instrumentation-ioredis/src/utils.ts	91.66% <0.00%> (+0.92%)	⬆️

[dyladan]

There used to be a semconv for the component attribute and it was removed. There were quite some discussions about this attribute in the spec at the time, but I think I would prefer to come to some specification consensus on this.

[blumamir]

I think I would prefer to come to some specification consensus on this.

It could be great. Do you mean the general opentelemetry specification? or convention only for the javascript?

[dyladan]

I think I would prefer to come to some specification consensus on this.

It could be great. Do you mean the general opentelemetry specification? or convention only for the javascript?

I mean the opentelemetry specification

[blumamir]

I think I would prefer to come to some specification consensus on this.

It could be great. Do you mean the general opentelemetry specification? or convention only for the javascript?

I mean the opentelemetry specification

open-telemetry/opentelemetry-specification#1605

[blumamir]

I think it might take time for this feature to enter spec.
@dyladan in the meantime, if I'll change this PR to send the module version via request hook, so instrumentation user can add it as span attribute under whatever attribute name he desires, will it be ok for you?

[dyladan]

@blumamir sorry for slow response i was out for part of last week.

in the meantime, if I'll change this PR to send the module version via request hook, so instrumentation user can add it as span attribute under whatever attribute name he desires, will it be ok for you?

I'm not sure what exactly you mean by this. The current implementation looks fine to me and I was about to approve actually (pending broken tests) before I saw this comment.

Do you mean like this?

class IoredisInstrumentation extends ... {
  // ...

  // register a hook to add custom attributes to spans
  onRequest((req: Request, span: Span) => void): void;
}

[blumamir]

Do you mean like this?

I was thinking to add it to the hook function parameters like this:

export interface IORedisInstrumentationConfig extends InstrumentationConfig {
  requestHook?: (span: Span, ioredisVersion: string, /** other params */) => void;
}

and then instrumentation user can write hook that adds the span attribute under whatever name he desires:

new IORedisInstrumentation({
    requestHook: (span: Span, ioredisVersion: string) => {
         span.setAttribute('whatever.name.user.want', ioredisVersion);
    }
});

Hope the syntax is right, I wrote it here without validating it.

I'm ok with both the moduleVersionAttributeName approach and the requestHook approach. Whichever one you think is better.

[obecny]

Would actually make sense to make it on instrumentation level a general hook ?.
Maybe something like onSpanStarted where we could pass an object param with information about instrumented library name and version ? There are more and more instrumentation where people want to add some additional information, it doesn't has to work in every edge case, but majority of usage will be to just add something to the span and I would like NOT to do this for every single instrumentation.

[dyladan]

Would actually make sense to make it on instrumentation level a general hook ?.
Maybe something like onSpanStarted where we could pass an object param with information about instrumented library name and version ? There are more and more instrumentation where people want to add some additional information, it doesn't has to work in every edge case, but majority of usage will be to just add something to the span and I would like NOT to do this for every single instrumentation.

Think this is quite dependent on each instrumentation I think in terms of what would be useful (http instrumentation hook might need the request object, mysql hook might need the query and the connection). If only the span is sent to the hook, it might not be enough information for the user to add the things that are important to them.

[obecny]

whoops @dyladan I think I edited yr response, fixed :/

Think this is quite dependent on each instrumentation I think in terms of what would be useful (http instrumentation hook might need the request object, mysql hook might need the query and the connection). If only the span is sent to the hook, it might not be enough information for the user to add the things that are important to them.

Yea but we can think of general implementation in instrumentation

onSpanStarted(span, somegeneralParams)

and if you need more information from certain instrumentation then extend it

onSpanStarted(span, someGeneralParams, someAdditionalParams....)

I'm not saying we have to do it now, but to avoid adding more less the same to many instrumentations we can think if creating some general and simply add more info in specific instrumentations. The advantage would be that you will have one hook that exists always, so it will be easier to unify things

[dyladan]

I think it most likely can be done in a mostly general way

[blumamir]

@obecny - if we can generalize it that would be awesome.
How do you imagine the general invocation of a hook when the instrumentation is patching multiple different modules, or patch the same module but with multiple installed versions?

In the past, I was experimenting with collecting stack trace per span (to help correlate the span to the code), and I did it with "require-in-the-middle" patching the tracer startSpan function. For those use cases adding a general start span hook can be very useful as well.

[obecny]

startSpan

Maybe create some observer / subscriber / interceptor solution (in tracer / provider) that would allow to make such things. In the worst scenario basically patching our tracer startSpan, but I didn't analyse it deeply yet. I know this can be done and could be quite useful for end users.

[obecny]

@blumamir with regards to

new IORedisInstrumentation({
    requestHook: (span: Span, ioredisVersion: string) => {
         span.setAttribute('whatever.name.user.want', ioredisVersion);
    }
});

I think this is the best approach I would just change one thing

new IORedisInstrumentation({
    requestHook: (span: Span, hookInfo: IoRedisHookInformation) => {
         span.setAttribute('whatever.name.user.want', hookInfo.ioredisVersion);
    }
});

The reason is that it will be easier to add / change things in future and to avoid having too many params being added as next param. I can imagine someone else would be willing to export more data one day.

[blumamir]

@blumamir with regards to

new IORedisInstrumentation({
    requestHook: (span: Span, ioredisVersion: string) => {
         span.setAttribute('whatever.name.user.want', ioredisVersion);
    }
});

I think this is the best approach I would just change one thing

new IORedisInstrumentation({
    requestHook: (span: Span, hookInfo: IoRedisHookInformation) => {
         span.setAttribute('whatever.name.user.want', hookInfo.ioredisVersion);
    }
});

The reason is that it will be easier to add / change things in future and to avoid having too many params being added as next param. I can imagine someone else would be willing to export more data one day.

@obecny Cool I think it's a good approach as well. Thank you for reviewing.
I'll update the PR next week as I'm having very busy days.

[blumamir]

@obecny - Update the PR to work with hooks like you suggested

[blumamir]

The failing checks are not related to this PR

[obecny]

lgtm, just the name of interface should start with IO not the Io, the rest are minor

[obecny]

lgtm, thx for changes