-
Notifications
You must be signed in to change notification settings - Fork 518
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
improv(ioredis): require parent span #42
improv(ioredis): require parent span #42
Conversation
5b412ce
to
2a15307
Compare
cc @naseemkullah since you wrote the plugin |
Would this prevent background queue processing tasks from emitting spans? Not that that's necessarily bad |
Codecov Report
@@ Coverage Diff @@
## master #42 +/- ##
==========================================
+ Coverage 94.49% 94.51% +0.01%
==========================================
Files 62 62
Lines 3509 3520 +11
Branches 371 372 +1
==========================================
+ Hits 3316 3327 +11
Misses 193 193
|
Well yes but if there aren't created inside a trace it doesn't make sense to trace these i believe. |
LGTM |
2a15307
to
ff83f92
Compare
ff83f92
to
1820275
Compare
@open-telemetry/javascript-approvers bump @vmarchaud Could you kindly resolve conflicts? |
035facb
to
05e398e
Compare
I've rebased the PR |
@open-telemetry/javascript-maintainers would it be possible to include this one in the next cut? |
In the next contrib cut sure, but we're going to wait for 0.9.0 of the core first |
Great, thanks! |
5fc5ae9
to
7439a93
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.
LGTM
7439a93
to
56c4845
Compare
I've rebased and bumped again the types, i think its good to merge cc @dyladan @mayurkale22 |
I've also fixed a missing dep in
test-utils
which was making my local install fail.