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

feat: add OpenTracing example #581

Merged
merged 7 commits into from
Dec 18, 2019

Conversation

hieven
Copy link
Contributor

@hieven hieven commented Dec 2, 2019

Which problem is this PR solving?

Short description of the changes

  • Add an example for how to use shim-opentracing

@codecov-io
Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #581 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
- Coverage   90.45%   90.44%   -0.01%     
==========================================
  Files         181      177       -4     
  Lines        9166     8990     -176     
  Branches      815      787      -28     
==========================================
- Hits         8291     8131     -160     
+ Misses        875      859      -16
Impacted Files Coverage Δ
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 66.66% <0%> (-33.34%) ⬇️
...res/opentelemetry-plugin-pg/test/assertionUtils.ts 96.29% <0%> (-3.71%) ⬇️
...core/src/context/propagation/BinaryTraceContext.ts 96.72% <0%> (-1.62%) ⬇️
...opentelemetry-base/test/resources/resource.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/config.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/src/utility.ts 100% <0%> (ø) ⬆️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100% <0%> (ø) ⬆️
...telemetry-plugin-grpc/test/utils/assertionUtils.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-tracing/test/Span.test.ts 100% <0%> (ø) ⬆️
.../opentelemetry-plugin-dns/test/utils/assertSpan.ts 100% <0%> (ø) ⬆️
... and 25 more

examples/opentracing-shim/README.md Outdated Show resolved Hide resolved
examples/opentracing-shim/client.js Outdated Show resolved Hide resolved

const opentracing = require("opentracing");

function init(serviceName) {
Copy link
Member

Choose a reason for hiding this comment

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

The fact that this file does OpenTracing and OpenTelemetry things together might be confusing, especially since the names are so similar. I would rather have one file that sets up open tracing the way you would normally set it up with open tracing, and another file called something like shim.js that sets up the shim. This way the example more clearly shows where the shim happens.

// shim.js

exports.shim = function shim(serviceName) {
  const tracer = new NodeTracer();
  tracer.addSpanProcessor(new SimpleSpanProcessor(getExporter()));

  return new TracerShim(tracer);
}

function getExporter() {
  const type = process.env.EXPORTER || "jaeger";
  if (type.match(/^z/)) {
    return new ZipkinExporter({ serviceName });
  }
  return new JaegerExporter({ serviceName, flushInterval: 100 });
}
// client.js
const shim = require("./shim").shim("http_client_service");

opentracing.initGlobalTracer(shim);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was initially imagining people usually have a tracer package which does initialization for open tracing and vendor specific instance (e.g. Zipkin or Jaeger). In this case, open telemetry is just another vendor so I made this decision to put all of them together.

I'm wondering if this makes sense to you and if adding some comments in the code can help or it will still be better to be separated as you suggest?

Many thanks for the suggestion! 👍

Copy link
Member

Choose a reason for hiding this comment

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

I think i can go either way. I would still rather have the shim separate from the opentracing tracer so it is clear where the break is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, as your suggestion, I've created a shim file to make thing more clear. Thanks!

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.

Great! Added few comments.

examples/opentracing-shim/README.md Outdated Show resolved Hide resolved
examples/opentracing-shim/README.md Show resolved Hide resolved
examples/opentracing-shim/package.json Outdated Show resolved Hide resolved
examples/opentracing-shim/server.js 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 PR, added a few comments.

examples/opentracing-shim/README.md Show resolved Hide resolved
examples/opentracing-shim/package.json Outdated Show resolved Hide resolved
examples/opentracing-shim/tracer.js Outdated Show resolved Hide resolved
examples/opentracing-shim/utils.js Show resolved Hide resolved
examples/opentracing-shim/server.js Outdated Show resolved Hide resolved
examples/opentracing-shim/client.js Outdated Show resolved Hide resolved
- Combine `Tracer` and `.init` to one line
- Use `setTimeout` to be consistent with other example
- Remove unused dependency
- Add missing keywords
- Update README
- Add missing `"use strict";`
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, added one comment.

examples/opentracing-shim/README.md Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

@open-telemetry/javascript-approvers: We need more reviews for this one!

@mayurkale22 mayurkale22 added document Documentation-related Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) and removed Awaiting reviewer feedback labels Dec 10, 2019
@mayurkale22
Copy link
Member

@hieven please rebase with the master, looks good to merge.

@mayurkale22 mayurkale22 merged commit af5a364 into open-telemetry:master Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
document Documentation-related Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OpenTracing shim example
5 participants