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(web-exception-instrumentation): Add instrumentation for web exceptions #2751

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

pkanal
Copy link
Contributor

@pkanal pkanal commented Mar 10, 2025

Which problem is this PR solving?

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:

  • Track runtime errors in production applications
  • Understand the frequency and types of errors occurring
  • Collect detailed error information including stack traces
  • Add custom attributes to error events

Short description of the changes

  • Captures both synchronous errors and unhandled promise rejections
  • Records error name, message, and stack trace using OpenTelemetry semantic conventions
  • Supports custom attribute callback for extending error information
  • Uses the OpenTelemetry Events API for lightweight error reporting
  • Maintains proper cleanup of event listeners

Open questions

  • This instrumentation probably shouldn't rely on the events API directly, should the instrumentation class also expose an events method like it does for tracing and logging?
  • There is also a dependency on types from the api and api-logs package, is that okay or should they be redefined?

Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.32%. Comparing base (b520d04) to head (a37abc4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2751      +/-   ##
==========================================
- Coverage   91.53%   91.32%   -0.21%     
==========================================
  Files         171      166       -5     
  Lines        8172     7861     -311     
  Branches     1660     1619      -41     
==========================================
- Hits         7480     7179     -301     
+ Misses        692      682      -10     

see 6 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

name: 'exception',
data: { ...errorAttributes, ...customAttributes },
severityNumber: SeverityNumber.ERROR,
timestamp: Date.now(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

: {};

eventLogger.emit({
name: 'exception',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Event names should be namespaced so just exception doesn't actually match the semantic conventions.

Please work with @Karlie-777 and her PR around defining this exception open-telemetry/semantic-conventions#1942. I agree that using the 3 top level attributes makes sense for these and I've left some comments in the link PR as well.

There are a few other properties that we should also be setting (if possible), which does mean attempting to parse out the exception on the client.


eventLogger.emit({
name: 'exception',
data: { ...errorAttributes, ...customAttributes },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be including the fully qualified "named" attributes in the payload (data), they should just be passed as attributes (or if we are not using the top-level attributes then they should not be prefixed by "exception.")

[ATTR_EXCEPTION_STACKTRACE]: error.stack,
};

const eventLogger = events.getEventLogger(
Copy link
Contributor

@MSNev MSNev Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also be including the "parsed" filename as well as the current url (hosting page), I also left a comment in the related event PR that this should be part of the client exception

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: if we end up using the url.full then to comply with this PR it MUST be redacted and I don't believe that the JS repo currently has any "helpers" to construct a redacted url.full.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comment, while it would be "nice" to just say that the URL is in the "resources" for SPA's it really needs to be mutable which resources can't currently be 😢

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

Successfully merging this pull request may close these issues.

3 participants