-
Notifications
You must be signed in to change notification settings - Fork 821
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: zipkin in web #1265 #1393
Conversation
refactored for browser compatibility in addition to existing nodejs closes issue open-telemetry#1265
|
Codecov Report
@@ Coverage Diff @@
## master #1393 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 149 149
Lines 4322 4322
Branches 880 880
=======================================
Hits 4064 4064
Misses 258 258
|
@@ -4,7 +4,10 @@ | |||
"description": "OpenTelemetry Zipkin Exporter allows the user to send collected traces to Zipkin.", | |||
"main": "build/src/index.js", | |||
"types": "build/src/index.d.ts", | |||
"repository": "open-telemetry/opentelemetry-js", | |||
"repository": "open-telemetry/opentelemetry-js", "browser": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser on its own line please.
Why is this a different platform configuration style than our other packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed for the new line.
I don't understand the issue with the "different style than the other packages", could you elaborate please ? I took @opentelemetry/exporter-collector as an exemple.
@@ -0,0 +1 @@ | |||
export * from './zipkin'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing header
please add a newline at the end of the file
/** | ||
* Zipkin Exporter | ||
*/ | ||
export class ZipkinExporter implements SpanExporter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost the same as the node version, but is completely copied. Is there no way to use a base class to share functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should refactor zipkin in a way that only send
method should be extracted and defined in platform. For web you should use XMLHttpRequest
not fetch
but default method should be sendBeacon
with a fallback to XMLHttpRequest
if user defines any headers. You can check the ExporterCollector
how it is done and I think it should have similar styling / approach here, thx
Have you checked that the issue you are trying to resolve has been assigned ? |
Yes, I did see it was assigned to you @obecny but I thought I could help. I hope this doesn't go against the contributing guidelines. I must admit I'm not very familiar with collaboration so I'm sorry if I crossed any line. |
I was working on this already and almost finished |
Sorry I overstepped (even if I'm happy I did because I'm learning doing so :)), I guess your PR will probably be way more qualitative. I'll probably try to implement the improvements you and dyladan raised, just for my own sake, but this PR can bit ignored/closed I assume. |
No worries :), we are always happy to welcome people who wants to contribute. There are many opened issues which are not assigned to anyone, maybe you could have a look at them and see if anything sounds interesting for you (don't feel being pushed) and if so you can comment on that issue so we can assign it to you to avoid duplication of work. |
Co-authored-by: Haddas Bronfman <[email protected]>
Which problem is this PR solving?
Zipkin in web #1265,
Short description of the changes
Refactored for browser compatibility in addition to nodejs
/!\ Lacks testing