Skip to content
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

Merged

Conversation

vmarchaud
Copy link
Member

I've also fixed a missing dep in test-utils which was making my local install fail.

@vmarchaud
Copy link
Member Author

cc @naseemkullah since you wrote the plugin

@vmarchaud vmarchaud added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 30, 2020
@naseemkullah
Copy link
Member

Would this prevent background queue processing tasks from emitting spans? Not that that's necessarily bad

@codecov
Copy link

codecov bot commented May 30, 2020

Codecov Report

Merging #42 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            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              
Impacted Files Coverage Δ
.../opentelemetry-plugin-ioredis/test/ioredis.test.ts 95.30% <0.00%> (+0.14%) ⬆️
node/opentelemetry-plugin-ioredis/src/utils.ts 85.36% <0.00%> (+0.75%) ⬆️

@vmarchaud
Copy link
Member Author

Would this prevent background queue processing tasks from emitting spans? Not that that's necessarily bad

Well yes but if there aren't created inside a trace it doesn't make sense to trace these i believe.
If you want to trace a background execution, you would need to create a span and then launch your task inside a withSpan.

@naseemkullah
Copy link
Member

LGTM

@vmarchaud vmarchaud force-pushed the improv/io-redis-require-parent branch from 2a15307 to ff83f92 Compare May 30, 2020 16:11
@vmarchaud vmarchaud force-pushed the improv/io-redis-require-parent branch from ff83f92 to 1820275 Compare May 30, 2020 16:15
@naseemkullah
Copy link
Member

@open-telemetry/javascript-approvers bump

@vmarchaud Could you kindly resolve conflicts?

@vmarchaud
Copy link
Member Author

I've rebased the PR

@naseemkullah
Copy link
Member

@open-telemetry/javascript-maintainers would it be possible to include this one in the next cut?

@dyladan
Copy link
Member

dyladan commented Jun 18, 2020

In the next contrib cut sure, but we're going to wait for 0.9.0 of the core first

@naseemkullah
Copy link
Member

In the next contrib cut sure, but we're going to wait for 0.9.0 of the core first

Great, thanks!

@vmarchaud vmarchaud force-pushed the improv/io-redis-require-parent branch from 5fc5ae9 to 7439a93 Compare June 18, 2020 16:10
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vmarchaud vmarchaud force-pushed the improv/io-redis-require-parent branch from 7439a93 to 56c4845 Compare June 23, 2020 20:04
@vmarchaud
Copy link
Member Author

vmarchaud commented Jun 23, 2020

I've rebased and bumped again the types, i think its good to merge cc @dyladan @mayurkale22

@dyladan dyladan merged commit fd1d39b into open-telemetry:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants