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(plugin-http): add example #318

Conversation

OlivierAlbertini
Copy link
Member

@OlivierAlbertini OlivierAlbertini commented Sep 22, 2019

Which problem is this PR solving?

Short description of the changes

  • Provides this example in order to facilitate testing for people that would like to play around.
  • See comments (where I'm facing to some situations, I will create issues).

I will remove the draft status once PRs will be merged:

Working with Zipkin

See Files changed section (image)
And below attributes:

Capture d’écran, le 2019-09-24 à 15 27 01
Capture d’écran, le 2019-09-24 à 15 27 17

Working with Jaeger

jaeger-ui

@OlivierAlbertini OlivierAlbertini force-pushed the feature/add-http-plugin-example branch 2 times, most recently from ade6b7d to 36509bd Compare September 22, 2019 20:16
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.

Nice!

examples/http/README.md Outdated Show resolved Hide resolved
examples/http/server.js Outdated Show resolved Hide resolved
examples/http/setup.js Outdated Show resolved Hide resolved
examples/http/client.js Outdated Show resolved Hide resolved
examples/http/setup.js Outdated Show resolved Hide resolved
// need ignoreOutgoingUrls: [/spans/] to avoid infinity loops
// TODO: manage this situation
const zipkinExporter = new ZipkinExporter(options);
exporter = new SimpleSpanProcessor(zipkinExporter);
Copy link
Member

Choose a reason for hiding this comment

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

s/exporter/processor/g ?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think we should use BatchSpanProcessor in case of Zipkin exporter (dependency on #238). Jaeger library has its own batching mechanism, so it makes sense to use SimpleSpanProcessor with Jaeger exporter.

@OlivierAlbertini OlivierAlbertini force-pushed the feature/add-http-plugin-example branch 2 times, most recently from 6aeb847 to 00d6798 Compare September 24, 2019 13:17
@mayurkale22
Copy link
Member

@OlivierAlbertini Looks like all the dependent PRs are merged, Could you please change this to Ready to Review.

examples/http/setup.js Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

Not working with Jaeger (don't see spans in Jaeger UI) Do I need to tweak the exporter config ?

I will give it a try on Jaeger exporter.

@mayurkale22
Copy link
Member

@OlivierAlbertini If you want, you can use this image for the Jaeger exporter example. I did some hacking to generate this with your example. I will add agenda for tomorrow's SIG meeting to discuss about this.

Jaeger_exporter

@codecov-io
Copy link

codecov-io commented Sep 25, 2019

Codecov Report

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

@@           Coverage Diff            @@
##           master   #318      +/-   ##
========================================
+ Coverage   97.97%    98%   +0.02%     
========================================
  Files         101    101              
  Lines        4891   4912      +21     
  Branches      413    415       +2     
========================================
+ Hits         4792   4814      +22     
+ Misses         99     98       -1
Impacted Files Coverage Δ
...telemetry-scope-base/test/NoopScopeManager.test.ts 100% <0%> (ø) ⬆️
...ages/opentelemetry-plugin-http/test/utils/Utils.ts 50% <0%> (+21.42%) ⬆️

@mayurkale22
Copy link
Member

Ping, for more reviews.

@OlivierAlbertini OlivierAlbertini force-pushed the feature/add-http-plugin-example branch from 3311b22 to 93bd17c Compare September 25, 2019 19:45
@mayurkale22
Copy link
Member

@OlivierAlbertini Thanks for the example. #335 and #340 are not a real blocker for this one and let's keep SimpleSpanProcessor for this example to see the immediate export.

@mayurkale22 mayurkale22 merged commit af06a2d into open-telemetry:master Sep 25, 2019
lukaswelinder pushed a commit to agile-pm/opentelemetry-js that referenced this pull request Jul 24, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
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.

examples: provide an example for HTTP plugin
3 participants