-
Notifications
You must be signed in to change notification settings - Fork 500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(instr-knex): implement requireParentSpan config flag #2288
feat(instr-knex): implement requireParentSpan config flag #2288
Conversation
a95703b
to
6dbb1a6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
6dbb1a6
to
c5a9f39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
See also my comment at open-telemetry/opentelemetry-js#4788 (comment) |
@blumamir some responses inline:
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.
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
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 |
This is perhaps getting off-track for this issue, but I wonder if the following would work for the use case:
Anyway, just musing. |
Which problem is this PR solving?
Many other instrumentations have a
requireParentSpan
flag, includinghttp
,undici
, andpg
, 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