Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

implement redis instrumentation plugin #359

Merged
merged 1 commit into from
Mar 3, 2019
Merged

implement redis instrumentation plugin #359

merged 1 commit into from
Mar 3, 2019

Conversation

vmarchaud
Copy link
Contributor

Not sure if everything is okay it terms of setup since i don't use lerna a lot, same with circleCI.
I've set the version to 0.0.1 but i believe you guys have all the packages at the same version for now, feel free to do it as you want.
I also choose not to add it into the instrumentation-all packages directly too, but i could do another PR if the plugin in itself is good.

cc @draffensperger @mayurkale22

@codecov-io
Copy link

codecov-io commented Feb 24, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
- Coverage   95.23%   95.22%   -0.02%     
==========================================
  Files         129      131       +2     
  Lines        8592     8758     +166     
  Branches      634      638       +4     
==========================================
+ Hits         8183     8340     +157     
- Misses        409      418       +9
Impacted Files Coverage Δ
src/stackdriver-monitoring.ts 81.05% <0%> (-2.11%) ⬇️
src/redis.ts 93.54% <0%> (ø)
test/test-redis.ts 97.11% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd1a7b...425006b. Read the comment docs.

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks for taking the initiative on this!

I have a bunch of nit-picky comments, but overall this is great.

packages/opencensus-instrumentation-redis/src/redis.ts Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/src/redis.ts Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/src/redis.ts Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/tsconfig.json Outdated Show resolved Hide resolved
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.

Thanks for the contribution, I have added a few comments.

packages/opencensus-instrumentation-redis/package.json Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/package.json Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/src/index.ts Outdated Show resolved Hide resolved
packages/opencensus-instrumentation-redis/tsconfig.json Outdated Show resolved Hide resolved
@vmarchaud
Copy link
Contributor Author

I believe i addressed every comment, note that the test failures are related to Stackdriver and not the PR itself

@mayurkale22
Copy link
Member

Please rebase the PR, this should fix build issue.

@mayurkale22
Copy link
Member

This is good to go, please rebase once.

@vmarchaud
Copy link
Contributor Author

@mayurkale22 Done

@mayurkale22
Copy link
Member

Thanks for the contribution again! If possible, please include redis instrumentation example in examples package.

@mayurkale22 mayurkale22 merged commit d26ce56 into census-instrumentation:master Mar 3, 2019
@vmarchaud
Copy link
Contributor Author

Yes, no problem. I will try to make another PR with the support for ioredis, and add both in examples and in opencensus-instrumentation-all

@vmarchaud vmarchaud deleted the redis-plugin branch March 3, 2019 21:00
@mayurkale22 mayurkale22 added this to the Release 0.0.10 milestone Mar 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants