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

add redis instrumentation plugins by default #471

Conversation

Projects
None yet
4 participants
@vmarchaud
Copy link
Contributor

commented Apr 4, 2019

As mentioned before, adding a new plugin to the default list need 2 different global release currently:

  • the first one to push it to npm and add it as a dependency of the instrumentation-all module
  • and the second to publish the new version of instrumentation-all so the new plugin are enabled by default.

We might want to change that to release new plugins faster. How do you think we could do that ?

@codecov-io

This comment has been minimized.

Copy link

commented Apr 4, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
- Coverage   94.71%   94.55%   -0.16%     
==========================================
  Files         150      150              
  Lines        9800     9572     -228     
  Branches      737      726      -11     
==========================================
- Hits         9282     9051     -231     
- Misses        518      521       +3
Impacted Files Coverage Δ
src/detect-resource.ts 66.66% <0%> (-26.2%) ⬇️
src/common-utils.ts 75% <0%> (-20.84%) ⬇️
test/test-detect-resource.ts 93.84% <0%> (-5.68%) ⬇️
src/http-stats.ts 100% <0%> (ø) ⬆️
src/grpc-stats/server-stats.ts 100% <0%> (ø) ⬆️
src/resource-labels.ts 100% <0%> (ø) ⬆️
src/grpc-stats/client-stats.ts 100% <0%> (ø) ⬆️
src/grpc-stats/common-distributions.ts 100% <0%> (ø) ⬆️
src/grpc-stats/stats-common.ts 100% <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 40f7b53...973bf66. Read the comment docs.

@mayurkale22
Copy link
Collaborator

left a comment

Thanks for the contribution, It will be nice to see the working example for Redis and IORedis.

@mayurkale22

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

We might want to change that to release new plugins faster. How do you think we could do that ?

If it's really required, I can do the patch release. For now, I would suggest to update the CHANGELOG (this way users will know that it is still in unreleased version). WDYT?

@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Thanks for the contribution, It will be nice to see the working example for Redis and IORedis.

Do you want me to add some examples with the zipkin UI ?

We might want to change that to release new plugins faster. How do you think we could do that ?

If it's really required, I can do the patch release. For now, I would suggest to update the CHANGELOG (this way users will know that it is still in unreleased version). WDYT?

I would not mind for now, but i think in the long run users might be frustrated to wait 2 releases to be able to use new plugins. Do you want me to modify the CHANGELOG in this PR or open up another one ?

@mayurkale22

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Do you want me to add some examples with the zipkin UI ?

That would be awesome, thanks.

Do you want me to modify the CHANGELOG in this PR or open up another one ?

Updating CHANGELOG in this PR makes sense to me.

I think in the long run users might be frustrated to wait 2 releases to be able to use new plugins.

I think we can do in one release only, Once you add new plugin we can add into instrumentation-all module with 0.0.1 version. This is what I did for binaryformat package. If you do npm install on top/main package lerna will help you to bind local (unreleased) package dependency properly. No issues for build or running locally.

One caveat is that if you do npm install on individual package where you have added this new dependency, it will fail to find the package. The workaround is you should run npm install on parent package.

I hope you got me, otherwise let me know.

@mayurkale22 mayurkale22 added this to the Release 0.0.11 milestone Apr 4, 2019

@mayurkale22 mayurkale22 merged commit 7609340 into census-instrumentation:master Apr 8, 2019

29 checks passed

ci/circleci: node10 Your tests passed on CircleCI!
Details
ci/circleci: node11 Your tests passed on CircleCI!
Details
ci/circleci: node6 Your tests passed on CircleCI!
Details
ci/circleci: node8 Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
security/snyk - examples/grpc/package.json (mayurkale22) No manifest changes detected
security/snyk - package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-core/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-example-automatic-tracing/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-instana/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-jaeger/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-ocagent/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-prometheus/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-stackdriver/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-zipkin/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-exporter-zpages/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-instrumentation-all/package.json (mayurkale22) No new issues
Details
security/snyk - packages/opencensus-instrumentation-grpc/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-instrumentation-http/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-instrumentation-http2/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-instrumentation-https/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-instrumentation-mongodb/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-nodejs/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-propagation-b3/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-propagation-binaryformat/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-propagation-jaeger/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-propagation-stackdriver/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-propagation-tracecontext/package.json (mayurkale22) No manifest changes detected
security/snyk - packages/opencensus-resource-util/package.json (mayurkale22) No manifest changes detected
@vmarchaud

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Sorry i didn't had the time to update the PR as i was traveling back to France. I will open a new one

@mayurkale22

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

Sorry i didn't had the time to update the PR as i was traveling back to France. I will open a new one

np, I will do that for you in post release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.