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

chore: refactoring zipkin to be able to use it in web #1399

Merged
merged 2 commits into from
Sep 3, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Aug 6, 2020

Which problem is this PR solving?

Short description of the changes

  • Adding possibility of using Zipkin Exporter in web
  • Added example too

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #1399 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1399   +/-   ##
=======================================
  Coverage   93.81%   93.82%           
=======================================
  Files         153      154    +1     
  Lines        4756     4762    +6     
  Branches      951      951           
=======================================
+ Hits         4462     4468    +6     
  Misses        294      294           
Impacted Files Coverage Δ
...ackages/opentelemetry-exporter-zipkin/src/types.ts 100.00% <ø> (ø)
...elemetry-exporter-zipkin/src/platform/node/util.ts 100.00% <100.00%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 100.00% <100.00%> (ø)

@Booster2ooo
Copy link

Hello,

I see a little something that looks off, if the response is a bad request (status code 400), the export will still be marked as success.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good with one minor comment

@dyladan
Copy link
Member

dyladan commented Aug 28, 2020

@open-telemetry/javascript-approvers this has been around a while and is a fairly safe change that has been requested multiple times

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM

@dyladan dyladan merged commit a7b9e9d into open-telemetry:master Sep 3, 2020
@obecny obecny deleted the zipkin branch October 1, 2020 14:19
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
enhancement New feature or request feature-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zipkin in web
4 participants