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 browser extension #498

Merged
merged 57 commits into from Jul 2, 2021
Merged

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented May 21, 2021

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

  • Add browser-extension/ at the root level of the repository
  • Add the source code to build the extension

@svrnm svrnm requested a review from a team as a code owner May 21, 2021 14:28
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #498 (131c142) into main (526f948) will decrease coverage by 1.91%.
The diff coverage is 95.49%.

❗ Current head 131c142 differs from pull request most recent head 13b830b. Consider uploading reports for the commit 13b830b to get more accurate results

@@            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     
Impacted Files Coverage Δ
...er-extension-autoinjection/src/background/index.ts 0.00% <0.00%> (ø)
...extension-autoinjection/src/contentScript/index.ts 0.00% <0.00%> (ø)
...metry-browser-extension-autoinjection/src/types.ts 90.32% <90.32%> (ø)
...njection/src/instrumentation/WebInstrumentation.ts 97.22% <97.22%> (ø)
...rc/background/ProgrammaticContentScriptInjector.ts 100.00% <100.00%> (ø)
...ction/src/contentScript/InstrumentationInjector.ts 100.00% <100.00%> (ø)
...er-extension-autoinjection/test/background.test.ts 100.00% <100.00%> (ø)
...extension-autoinjection/test/contentScript.test.ts 100.00% <100.00%> (ø)
...tension-autoinjection/test/instrumentation.test.ts 100.00% <100.00%> (ø)
...etry-browser-extension-autoinjection/test/utils.ts 100.00% <100.00%> (ø)
... and 166 more

Copy link
Member

@obecny obecny left a 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.

  1. 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.
  2. Can you please add some screenshots how it looks so we can see it too ?

browser-extension/tsconfig.json Outdated Show resolved Hide resolved
browser-extension/tsconfig.json Outdated Show resolved Hide resolved
browser-extension/webpack.config.ts Outdated Show resolved Hide resolved
browser-extension/webpack.config.ts Outdated Show resolved Hide resolved
browser-extension/package.json Outdated Show resolved Hide resolved
browser-extension/src/types.ts Outdated Show resolved Hide resolved
browser-extension/src/ui/index.tsx Outdated Show resolved Hide resolved
browser-extension/src/ui/styles.ts Outdated Show resolved Hide resolved
browser-extension/src/utils/EventEmitterSpanExporter.ts Outdated Show resolved Hide resolved
browser-extension/src/utils/EventEmitterSpanExporter.ts Outdated Show resolved Hide resolved
@svrnm
Copy link
Member Author

svrnm commented May 26, 2021

Thanks for taking the time to look into this!

  1. 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.

Sure!

  1. Can you please add some screenshots how it looks so we can see it too ?

Yes, I can expand the "Usage" guide with some screenshots: "what to do" and "what to expect"

svrnm and others added 9 commits May 26, 2021 15:11
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
@dyladan
Copy link
Member

dyladan commented May 26, 2021

In general we try to name packages opentelemetry-category-name. opentelemetry-injection-browser-extension is opentelemetry-name-category. I would prefer the name opentelemetry-browser-extension-injection, opentelemetry-browser-extension-autoinjection, opentelemetry-browser-ext-injection or similar.

@svrnm
Copy link
Member Author

svrnm commented May 26, 2021

In general we try to name packages opentelemetry-category-name. opentelemetry-injection-browser-extension is opentelemetry-name-category. I would prefer the name opentelemetry-browser-extension-injection, opentelemetry-browser-extension-autoinjection, opentelemetry-browser-ext-injection or similar.

Ah okay, that makes sense! I'll change that.

@mhennoch
Copy link
Contributor

I'll try to take a look tomorrow, we actually have something similar but haven't gotten around upstreaming it.

@svrnm
Copy link
Member Author

svrnm commented May 27, 2021

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:

  • Is the gif adding value or distracting?
  • there is some "not-yet-ready" code for forwarding spans to the service worker, can this stay or should I keep it out until it is working? (I might need some help here to get the spans exported properly...)
  • Any other tracers/libraries/things I should include for people to play around with?

Thanks,

@vmarchaud
Copy link
Member

Is the gif adding value or distracting?

I think its worth adding gif to demo something thats visual, we sadly don't have a lot of visual things to demo generally ^^

there is some "not-yet-ready" code for forwarding spans to the service worker, can this stay or should I keep it out until it is working? (I might need some help here to get the spans exported properly...)

I would prefer to keep it out until everything works to avoid having people open issues for it

Copy link
Member

@vmarchaud vmarchaud left a 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

….json

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Copy link
Member

@obecny obecny left a 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 :)

@svrnm
Copy link
Member Author

svrnm commented Jun 24, 2021

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:-)

@vmarchaud vmarchaud requested a review from dyladan June 28, 2021 12:12
@svrnm
Copy link
Member Author

svrnm commented Jun 30, 2021

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.

@Flarna
Copy link
Member

Flarna commented Jun 30, 2021

I guess you have to add "codecov": "3.8.2" as dev dependency.

@svrnm
Copy link
Member Author

svrnm commented Jul 1, 2021

Thanks @Flarna! Now it works, @dyladan :-)

@vmarchaud vmarchaud merged commit 4e5b205 into open-telemetry:main Jul 2, 2021
@svrnm
Copy link
Member Author

svrnm commented Jul 2, 2021

Thanks everyone for making this happen 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants