-
Notifications
You must be signed in to change notification settings - Fork 615
feat(web-exception-instrumentation): Add instrumentation for web exceptions #2751
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(web-exception-instrumentation): Add instrumentation for web exceptions #2751
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main open-telemetry/opentelemetry-js-contrib#2751 +/- ##
=======================================
Coverage 89.60% 89.60%
=======================================
Files 174 174
Lines 8450 8450
Branches 1660 1660
=======================================
Hits 7572 7572
Misses 878 878 🚀 New features to boost your workflow:
|
plugins/web/opentelemetry-instrumentation-web-exception/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-web-exception/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-web-exception/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-web-exception/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
a37abc4 to
f1816f9
Compare
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'm not sure what version new instrumentation should have. Do we start out with 0.0.1?
- I don't love prefixing the name with Web but I know that is a longer conversation. Should we do that now or not? Or mark this as experimental in case we decide to remove Web/Browser prefixes as instrumentations migrates to the browser repo?
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 can safely delete this file.
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 can safely delete this file.
| "lint": "eslint . --ext .ts", | ||
| "lint:fix": "eslint . --ext .ts --fix", | ||
| "lint:readme": "node ../../scripts/lint-readme.js", |
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.
These are now handled at the root of the repo.
| "lint": "eslint . --ext .ts", | |
| "lint:fix": "eslint . --ext .ts --fix", | |
| "lint:readme": "node ../../scripts/lint-readme.js", |
|
Thanks for Reviewing @overbalance
I didn't see any guidance here. Maybe 0.1.0? I don't have strong opinions, other than it's not a 1,0, since it's still experimental to point 2 below.
I think it's experimental by default unless flagged otherwise, but we could make this explicit. I'm in favor of merging this if the implementation is acceptable and we can discuss the browser naming conventions, prefixes and disambiguation more wholistically at Browser SIG? |
|
@wolfgangcodes Agreed, we should merge first and discuss in the SIG meeting and on Slack. |
|
Looks good % package-lock conflicts. 🙂 |
Which problem is this PR solving?
Closes open-telemetry/opentelemetry-browser#14
Currently, there's no standardized way to capture and monitor unhandled exceptions and promise rejections in web applications using OpenTelemetry. This makes it difficult for developers to:
Short description of the changes
Open questions
apiandapi-logspackage, is that okay or should they be redefined?