feat(instr-knex): implement requireParentSpan config flag

[reberhardt7]

Which problem is this PR solving?

Many other instrumentations have a requireParentSpan flag, including http, undici, and pg, which helps to reduce tracing noise and load when we are only trying to instrument a small part of an existing system. This PR adds the flag for knex.

Short description of the changes

Adds a flag to avoid generating spans for knex queries if no parent span is present

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dfb2dff) to head (bd1622b).
Report is 195 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2288      +/-   ##
==========================================
- Coverage   90.97%   90.40%   -0.58%     
==========================================
  Files         146      149       +3     
  Lines        7492     7359     -133     
  Branches     1502     1527      +25     
==========================================
- Hits         6816     6653     -163     
- Misses        676      706      +30     

Files	Coverage Δ
...emetry-instrumentation-knex/src/instrumentation.ts	98.79% <100.00%> (+0.01%)	⬆️

... and 65 files with indirect coverage changes

[blumamir]

Thank you for opening the PR and your contribution.
The implementation looks good to me, and I understand the usecase and that this practice is already inmplemented in other instrumentations.

Having said that, I think it's worth mentioning here few of the past concerns with this config option:

• while requireParentSpan can untrace one specific span, users might still get spans for this execution if any downstream library is instrumented. Not sure how it works with knex, but if it uses a db client underneath then we can still get a span for that with non-complete context which requires yet another configuration.
• I think that the right place to control which spans are being untraced in opentelemetry is via samplers, and really wish we can find a practical way to move this logic there and deprecate all instrumentaiton config options that controls when a span is skipped and when not. unfortunately, since the samplers do not receive the instrumentation scope, this task becomes harder.

I am also curious if you can share a bit information about your motivation to create this PR - what is the untraced trigger in your service that makes knex calls without parent context?
is it from timers? does the context breaks somewhere along the way? are those heartbeats? is it your buisness logic or some framework? Did you observe cases in other instrumentations where you had to configure this option to reduce noise?

I wonder if we can solve the issue by supplying the context instead of ignoring the trace.

[trentm]

See also my comment at open-telemetry/opentelemetry-js#4788 (comment)

[reberhardt7]

@blumamir some responses inline:

I am also curious if you can share a bit information about your motivation to create this PR - what is the untraced trigger in your service that makes knex calls without parent context? is it from timers? does the context breaks somewhere along the way? are those heartbeats? is it your buisness logic or some framework? Did you observe cases in other instrumentations where you had to configure this option to reduce noise?

We were adding instrumentation to an existing complicated service, and I wanted to slowly add instrumentation to reduce the risk of degrading prod performance from suddenly generating lots of spans everywhere. At first, we only wanted to trace one specific request handler. We want database spans descending from that request handler, but not from any other handlers, which would be overwhelming.

Having said that, I think it's worth mentioning here few of the past concerns with this config option:

• while requireParentSpan can untrace one specific span, users might still get spans for this execution if any downstream library is instrumented. Not sure how it works with knex, but if it uses a db client underneath then we can still get a span for that with non-complete context which requires yet another configuration.

That's definitely a valid concern. We are using knex with pg, and did not add the @opentelemetry/instrumentation-pg auto-instrumentation since the knex instrumentation is sufficient. However, if we did enable that, it has a requireParentSpan option that we would enable as well. Not sure about other database clients.

• I think that the right place to control which spans are being untraced in opentelemetry is via samplers, and really wish we can find a practical way to move this logic there and deprecate all instrumentaiton config options that controls when a span is skipped and when not. unfortunately, since the samplers do not receive the instrumentation scope, this task becomes harder.

I think the sampler gets passed the context, so it could check whether there is a parent span that way, but I don't think it can see which instrumentation library or tracer generated the span. (Maybe that was what you were already pointing out.) I think writing a sampler is a much more general way to solve this problem, but it seems much simpler to get started tracing an existing app with a requireParentSpan flag.

[trentm]

This is perhaps getting off-track for this issue, but I wonder if the following would work for the use case:

• A custom Sampler (call it InterestingRequestPathsSampler) that returns true from shouldSample if the spanKind is SERVER (i.e. this is an incoming request) and the span attributes include a url.path [*] that matches one of a given set of "interesting" request paths.
• This sampler could be used in combination with ParentBasedSampler to respect an already set sampling decision on a parent span.

Anyway, just musing.