-
Notifications
You must be signed in to change notification settings - Fork 532
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: bring back browser extension #1152
Conversation
Signed-off-by: svrnm <neumanns@cisco.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1152 +/- ##
==========================================
+ Coverage 90.97% 96.13% +5.15%
==========================================
Files 146 14 -132
Lines 7492 906 -6586
Branches 1502 197 -1305
==========================================
- Hits 6816 871 -5945
+ Misses 676 35 -641 |
not sure how to fix the issues with node 8 & node 10. Best option would be to exclude them, since the final runtime is the browser and the testing doesn't need to run on all node versions. |
packages/opentelemetry-browser-extension-autoinjection/package.json
Outdated
Show resolved
Hide resolved
…ease please manifest
Failure appears to be a dependency conflict due to webpack versioning differences with other packages in the repo |
looks like I still have to learn about lerna and stuff... let me see if I can get this working somehow, because updating to a more recent version of webpack did the trick to get it compiled again, and downgrading sends me into a rabbit hole of dependency issues. |
@@ -26,60 +27,62 @@ | |||
"node": ">=14" | |||
}, | |||
"peerDependencies": { | |||
"@opentelemetry/api": "^1.0.0" | |||
"@opentelemetry/api": "^1.2.0" |
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.
Why upgrade the "@opentelemetry/api" minimum supported minor versions?
If the user is using an older version of the API, will it fail to work?
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.
Not sure why I changed it, it should work with 1.0 as well
Signed-off-by: svrnm <neumanns@cisco.com>
…e removed again) Signed-off-by: svrnm <neumanns@cisco.com>
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Still interesting in seeing this land 😀 anything I can do to help it along? #1486? it was closed without attention… |
Hey @SimenB, thanks for your interest in this! I was ooo the past few days and couldn't follow up. I think there are 2 options how to fix this, bumping webpack dependencies to 5. like in #1486 is the "better" solution, the other option is making the extension work differently (replace webpack with something else) |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Seems that other PR just needed |
wow, highly appreciated @SimenB ! |
Sorry on the delay on this. It's been several months since this was last updated, but @svrnm are you still interested in maintaining this? It seems like the blocker right now is to update web tests to use web-test-runner instead of karma. @SimenB already got the updated test runner in #1816 and there is an open PR with an error #2005. We'll try to take a look at #2005 either way and as a follow-up will want to get the rest of the web components updated to use web-test-runner instead of karma. But still it will be useful to know if you are still interested in maintaining this or if it's no longer a need right now. |
I am happy to maintain this, if the tests are unblocked now, or if this requires me to rewrite the tests with web-test-runner instead of karma I can look into that. |
Ok thanks for the confirmation! On #2005 we seem to have found the cause of the errors on updating to use the new test runner, so we should be able to keep moving forward on those. If that's an area you're familiar with it's always useful to have your input or feedback on those changes 😃 We'll probably want to add a commit or otherwise kick the CI to see the latest on lint and test results. |
We've hit some roadblocks with updating to |
I just merged #2269. The outdated webpack version should now not be a problem anymore 🙂 |
That's great news, @pichlermarc! I'll look into it eventually to fix the extension |
Signed-off-by: svrnm <neumanns@cisco.com>
Signed-off-by: svrnm <neumanns@cisco.com>
FYI, since the code hasn't been touched for that long it will take me some time to get everything back in place |
Note, that after discussing this with @JamieDanielson and @pichlermarc I will not bring back this browser extension to reduce the maintenance burden for the maintainers and myself. Instead I work towards listing existing extensions (like https://github.com/tbrockman/opentelemetry-browser-extension) on the Registry and then link out to the registry via the README of the archived extension. |
Which problem is this PR solving?
This PR brings back the browser extension by fixing the build errors & updating dependencies and some, see #1101, #1110
Short description of the changes
npm run compile
works againserver
folder that contains instructions on how to run a collector behind a nginx for proper CORS headers.