Skip to content

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

Open
wants to merge 18 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 89.60%. Comparing base (41d0d4b) to head (f1f5b06).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #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:
  • ❄️ 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.

: {};

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we be consistent with the existing semantic conventions for span events (where the event name is "exception")? Or, do we need to define exception event for each separate domain (e.g. "browser.exception", "android.exception" etc.)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to exception for now since we're not creating any client specific semantic conventions for errors yet. We can change it in the future if this pattern develops.

[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 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, since we're sticking with the existing semantic conventions instead of client specific ones I haven't included the parsed file name or URL. I think we can add the URL in a future PR and add some core helpers to do that separately.

@pkanal pkanal force-pushed the pkanal/browser-exception-instrumentation branch from a37abc4 to f1816f9 Compare April 22, 2025 15:34
@pkanal pkanal marked this pull request as ready for review April 22, 2025 15:37
@pkanal pkanal requested a review from a team as a code owner April 22, 2025 15:37
@github-actions github-actions bot requested a review from obecny April 22, 2025 15:37
@pkanal pkanal requested review from MSNev and martinkuba April 22, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants