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

docs: add ioredis example #665

Merged
merged 14 commits into from Jan 28, 2020

Conversation

naseemkullah
Copy link
Member

Which problem is this PR solving?

  • Adds an example for ioredis

Short description of the changes

  • Adds an example for ioredis

@codecov-io
Copy link

codecov-io commented Jan 5, 2020

Codecov Report

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

@@            Coverage Diff            @@
##           master    #665      +/-   ##
=========================================
- Coverage   92.61%     91%   -1.61%     
=========================================
  Files         228     225       -3     
  Lines       10248   10396     +148     
  Branches      934     956      +22     
=========================================
- Hits         9491    9461      -30     
- Misses        757     935     +178
Impacted Files Coverage Δ
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 63.39% <0%> (-36.61%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 63.63% <0%> (-36.37%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 65.06% <0%> (-34.94%) ⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts 71.42% <0%> (-28.58%) ⬇️
...ackages/opentelemetry-core/src/platform/node/id.ts 71.42% <0%> (-28.58%) ⬇️
... and 44 more

@dyladan
Copy link
Member

dyladan commented Jan 5, 2020

Nothing looks wrong to me here so I'm going to give it an approval, but in the future I think we should make the examples as simple as possible. This one has a client and server aspect to it and a lot of code not dealing specifically with ioredis which can make the ioredis code feel a little lost. I realize the other examples do this too, but there are a lot of examples now with client/server and I think we should simplify them.

Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

express and axios should be removed

@naseemkullah
Copy link
Member Author

Nothing looks wrong to me here so I'm going to give it an approval, but in the future I think we should make the examples as simple as possible. This one has a client and server aspect to it and a lot of code not dealing specifically with ioredis which can make the ioredis code feel a little lost. I realize the other examples do this too, but there are a lot of examples now with client/server and I think we should simplify them.

I agree. This example is based off of other examples in the repo as you mention but I will adjust it according to your suggestion.

@naseemkullah
Copy link
Member Author

naseemkullah commented Jan 6, 2020

Let's hold off as there is an inherent issue with the ioredis plugin in general as per #668

@naseemkullah
Copy link
Member Author

The example has been simplified.

@dyladan
Copy link
Member

dyladan commented Jan 6, 2020

Adding the hold label since we're waiting on another issue

@dyladan
Copy link
Member

dyladan commented Jan 23, 2020

@naseemkullah now that the bug is fixed, is this ready?

examples/ioredis/README.md Outdated Show resolved Hide resolved
@naseemkullah
Copy link
Member Author

@naseemkullah now that the bug is fixed, is this ready?

yep! thanks.

@dyladan dyladan added Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed do-not-merge/hold Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) labels Jan 24, 2020
Naseem added 2 commits January 26, 2020 23:24
Signed-off-by: Naseem <naseem@transit.app>
// Initialize the OpenTelemetry APIs to use the BasicTracer bindings
opentelemetry.initGlobalTracerRegistry(tracerRegistry);

module.exports = opentelemetry.getTracer();
Copy link
Member Author

Choose a reason for hiding this comment

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

@dyladan should this now be module.exports = opentelemetry.getTracerRegistry(); ?
Should the file be renamed to tracer-registry.js in this case?

As a side note I find this whole tracer registry/factory thing to be confusing for the end user. I wish the focus was that libs just not have broken implementations of tracers.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that means it becomes tracer registry.

"just don't have bugs" is not a feasible answer to the problem unfortunately.

Fortunately, most end users won't have to worry about it too much, just library developers. The only change for most end users is the name of the global object they need to create. Most users will never call any methods on the tracer registry manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for explaining.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dyladan should this now be module.exports = opentelemetry.getTracerRegistry(); ?
Should the file be renamed to tracer-registry.js in this case?

@dyladan I had not yet made the above fix. Do you want a follow up PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

You can make a follow up PR to handle it, but you're not actually using the exported value in your index, so it shouldn't matter.

@dyladan dyladan merged commit 3dd2a68 into open-telemetry:master Jan 28, 2020
dyladan added a commit that referenced this pull request Jan 28, 2020
@naseemkullah naseemkullah deleted the ioredis-example-2 branch January 28, 2020 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants