-
Notifications
You must be signed in to change notification settings - Fork 534
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 browser extension #498
Conversation
Codecov Report
@@ Coverage Diff @@
## main #498 +/- ##
==========================================
- Coverage 96.68% 94.77% -1.92%
==========================================
Files 13 179 +166
Lines 634 10921 +10287
Branches 124 1086 +962
==========================================
+ Hits 613 10350 +9737
- Misses 21 571 +550
|
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.
first pass, nice work, I have raised few issues.
- Please move also the package to folder
packages
for now, if we figure out better place it can be moved later, but in general I would not keep it in main folder. - Can you please add some screenshots how it looks so we can see it too ?
Thanks for taking the time to look into this!
Sure!
Yes, I can expand the "Usage" guide with some screenshots: "what to do" and "what to expect" |
Co-authored-by: Bartlomiej Obecny <[email protected]>
Co-authored-by: Bartlomiej Obecny <[email protected]>
Co-authored-by: Bartlomiej Obecny <[email protected]>
Co-authored-by: Bartlomiej Obecny <[email protected]>
Co-authored-by: Bartlomiej Obecny <[email protected]>
…y-js-contrib into browser-extension
…-contrib into browser-extension
In general we try to name packages |
Ah okay, that makes sense! I'll change that. |
I'll try to take a look tomorrow, we actually have something similar but haven't gotten around upstreaming it. |
…y-js-contrib into browser-extension
I applied most of the suggestions and added some questions above. The latest commit also adds some images (and a gif) to the README.md -- a few questions from my site:
Thanks, |
I think its worth adding gif to demo something thats visual, we sadly don't have a lot of visual things to demo generally ^^
I would prefer to keep it out until everything works to avoid having people open issues for it |
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.
overall lgtm, not a browser expert so i'll delegate actual approve to others
…y-js-contrib into browser-extension
…-contrib into browser-extension
…-contrib into browser-extension
packages/opentelemetry-browser-extension-autoinjection/package.json
Outdated
Show resolved
Hide resolved
….json Co-authored-by: Daniel Dyla <[email protected]>
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.
I think all my concerns have been addressed, meanwhile the new version has been released so I think it can be just upgraded to the latest. Gonna approve just please address upgrade to latest before merging, gr8 job :)
Thanks to @dyladan that's fixed already:-) |
Can someone help me to understand the code coverage issue? when I click into the details I get some errors. I guess I need to increase coverage on the extension, but I would like to verify. |
I guess you have to add |
Thanks everyone for making this happen 🎉 |
Which problem is this PR solving?
As discussed in #490, this PR provides code for a browser extension, that can be used to inject OpenTelemetry in an arbitrary website. The original repository at https://github.com/svrnm/opentelemetry-browser-extension/ is owned by me.
Short description of the changes