-
Notifications
You must be signed in to change notification settings - Fork 532
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(instrumentation-dataloader): Enhance dataloader
instrumentation.
#2449
base: main
Are you sure you want to change the base?
feat(instrumentation-dataloader): Enhance dataloader
instrumentation.
#2449
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2449 +/- ##
==========================================
- Coverage 90.97% 90.79% -0.19%
==========================================
Files 146 156 +10
Lines 7492 7766 +274
Branches 1502 1589 +87
==========================================
+ Hits 6816 7051 +235
- Misses 676 715 +39
|
30b8494
to
3529ca6
Compare
3529ca6
to
823281b
Compare
@@ -242,10 +250,139 @@ export class DataloaderInstrumentation extends InstrumentationBase<DataloaderIns | |||
// .loadMany never rejects, as errors from internal .load | |||
// calls are caught by dataloader lib | |||
return original.call(this, ...args).then(value => { | |||
span.setAttribute('cache.key', Array.from(args[0])); | |||
span.setAttribute( | |||
'cache.hit', |
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.
Unfortunately these are not official OTEL semantic conventions, so I think we cannot add these here. I think we'll need requestHook
and add these ourselves. For now we should remove this.
There are no cache conventions for OTEL yet. We're thinking about donating our conventions upstream for this.
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.
@AbhiPrasad - Should we add the requestHook
with these updates, or in a separate PR?
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.
Let's add requestHook
in a separate PR
Which problem is this PR solving?
prime
,clear
andclearAll
methods ofdataloader
Short description of the changes
Related: getsentry/sentry-javascript#13664 and getsentry/sentry-javascript#13724
The current implementation of dataloader instrumentation creates spans for only
load
,loadMany
andbatch
operations. With this PR, the instrumentation starts creating spans for synchronous 'non-read' operations,prime
,clear
, andclearAll
.