-
Notifications
You must be signed in to change notification settings - Fork 595
Replace @sentry/browser with generic internal error capture implementation #3226
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: yurishkuro <[email protected]>
|
@copilot I see you did 1 to 1 equivalence, but the functionality of IException and Breadcrumb is only used in conv-sentry-to-ga.tsx, so this just introduces extra indirection. Can we not simply call error-capture.tsx from tracking/ga.tsx directly and only implement minimal functionality that is actually required by ga.tsx? |
…to-ga Co-authored-by: yurishkuro <[email protected]>
You're right - I've simplified the implementation:
The types are now defined directly in conv-sentry-to-ga.tsx, and error-capture.tsx provides only the runtime functionality that ga.tsx requires. This eliminates the extra indirection while maintaining all functionality. Commit: 74dd64e |
|
@copilot This is still too much code. You have packages/jaeger-ui/src/utils/tracking/error-capture.tsx defining some types, then packages/jaeger-ui/src/utils/tracking/conv-sentry-to-ga.tsx converting them to some other types, then packages/jaeger-ui/src/utils/tracking/ga.tsx doing yet another conversion to package the data. Can we not remove conv-sentry-to-ga.tsx completely, and in error-capture.tsx define only the pieces of data that ga.tsx actually needs? |
…r-capture.tsx Co-authored-by: yurishkuro <[email protected]>
Completely removed conv-sentry-to-ga.tsx and integrated all conversion logic directly into error-capture.tsx. The error capture now:
Removed files:
The data flow is now direct: error-capture.tsx formats errors → ga.tsx tracks to GA. No intermediate conversions. Commit: 8c72e63 |
|
@copilot please go critically through ga.tsx and error-capture.tsx and make sure only needed functions are included in error-capture.tsx. For example, ga.tsx imports breadcrumbsIntegration but never uses it. |
Co-authored-by: yurishkuro <[email protected]>
Reviewed and removed unnecessary code:
All exports from error-capture.tsx are now actually used by ga.tsx:
Removed 9 lines of unnecessary wrapper code. Commit: f731704 |
|
@copilot in formatErrorForGA function, we first parsee stacktrace then format it again. Is that really necessary? Does GA accept some kind fo structured representation of a stack frame or just a string? If a string, why not just render the exception as it would be rendered if logged to console and submit it like that? const frames = parseStackTrace(error); |
Co-authored-by: yurishkuro <[email protected]>
You're right - the parse-then-format approach was unnecessary. GA only accepts strings, not structured data. Simplified stack trace handling:
Removed:
The new Reduced error-capture.tsx from 521 to 482 lines (39 lines saved). Commit: 0e62d6a |
|
|
||
| // Configuration options | ||
| interface IInitOptions { | ||
| dsn?: string; // Ignored, kept for API compatibility |
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.
there is no compatibility requirements, delete this
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.
Removed dsn option - no compatibility requirements needed.
Commit: 85d6ecf
| breadcrumbs?: IBreadcrumbConfig; | ||
| } | ||
|
|
||
| interface IBreadcrumbConfig { |
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.
there are no variations or customizations we require, remove this and just hardcode the settings and/or behavior
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.
Removed IBreadcrumbConfig interface and hardcoded the breadcrumb tracking settings (fetch + DOM events).
Commit: 85d6ecf
| // Minimal error capture that formats errors for Google Analytics | ||
|
|
||
| // GA-formatted error data | ||
| export interface IGAErrorData { |
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.
this module should not be GA aware, in theory we could use it with non-GA implementations.
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.
Made error-capture.tsx tracking-implementation agnostic. Removed IGAErrorData and replaced with generic IErrorData and IErrorContext interfaces. Module can now be used with non-GA implementations.
Commit: 85d6ecf
| } | ||
|
|
||
| // Format error for Google Analytics | ||
| function formatErrorForGA(error: Error): IGAErrorData { |
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.
this function should be in ga.tsx module if it's specific to GA
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.
Moved formatErrorForGA() and all GA-specific formatting functions (formatStack, formatBreadcrumbs, convErrorMessage, etc.) to ga.tsx module.
Commit: 85d6ecf
| }); | ||
| } | ||
|
|
||
| // Mock BrowserClient for type compatibility |
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.
There are no compatiblity requirements. REmove if not needed.
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.
Removed BrowserClient mock class - not needed without compatibility requirements.
Commit: 85d6ecf
| // Copyright (c) 2025 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Minimal error capture that formats errors for Google Analytics |
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.
"minimal" is quite a stretch. Write a header comment documenting what each function does and show examples of how it helps in reporting data for tracking.
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.
Added comprehensive header documentation with:
- Module purpose and features
- Key capabilities (error capture, breadcrumb tracking, context)
- Usage examples showing how to integrate with analytics
Commit: 85d6ecf
|
@copilot address all review comments in the PR |
…tting to ga.tsx Co-authored-by: yurishkuro <[email protected]>
Addressed all review comments:
error-capture.tsx is now a generic 262-line module that can be used with any analytics implementation. Commit: 85d6ecf |
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { BrowserClient } from '@sentry/browser'; | ||
| import { BrowserClient } from '../utils/tracking/error-capture'; |
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.
BrowserClient is not defined in error-capture
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.
Removed BrowserClient import - changed context type to boolean | null since BrowserClient is no longer exported.
Commit: a9d235c
| return UNKNOWN_SYM; | ||
| } | ||
|
|
||
| function convErrorMessage(message: string, maxLen = 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 is this function here instead of error-capture? it's not specific to GA
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.
Moved formatErrorMessage() to error-capture.tsx as it's generic (removes "Error:" prefix, cleans up formatting) - not GA-specific.
Commit: a9d235c
| .replace(/ > /g, ' >'); | ||
| } | ||
|
|
||
| function formatStack(stack: string | undefined): string { |
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 is this function here instead of error-capture? it's not specific to GA
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.
Moved formatStackTrace() to error-capture.tsx as the core logic (cleaning whitespace) is generic. Created formatStackForGA() in ga.tsx that adds Jaeger-specific path removal (origin, /static/js/).
Commit: a9d235c
| return lines.join('\n'); | ||
| } | ||
|
|
||
| function formatBreadcrumbs(crumbs: IBreadcrumb[]): string { |
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 is this function here instead of error-capture? it's not specific to GA
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.
formatBreadcrumbs() is GA/Jaeger-specific because it:
- Uses Jaeger-specific route symbols (NAV_SYMBOLS, FETCH_SYMBOLS for /search, /trace, /api/traces, etc.)
- Compresses breadcrumbs to fit GA's size limits
- Uses application-specific compression logic (compressCssSelector for Jaeger UI selectors)
Keeping it in ga.tsx as it's tightly coupled to Jaeger routes and GA requirements.
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.
it is Jaeger specific but not GA specific - we support alternative tracking solutions besides GA.
|
@copilot address code review comments |
…rove documentation Co-authored-by: yurishkuro <[email protected]>
Addressed all code review comments:
error-capture.tsx now exports generic utilities that can be used by any analytics implementation. Commit: a9d235c |
Summary
Successfully replaced the
@sentry/browserdependency with a generic internal error capture implementation. The implementation is tracking-agnostic and can be used with any analytics service, not just Google Analytics.Changes Made
formatErrorMessage,formatStackTrace)Architecture
error-capture.tsx (336 lines) - Generic error capturing module:
formatErrorMessage(),formatStackTrace()ga.tsx (430 lines) - Google Analytics implementation:
Key Changes
Removed files:
conv-sentry-to-ga.tsx(375 lines)conv-sentry-to-ga.test.js(12 lines)fixtures.ts(211 lines)Removed unnecessary code:
dsn,environmentoptions - no compatibility requirementsIBreadcrumbConfiginterface - hardcoded settings (fetch + DOM events)BrowserClientmock class - not neededbreadcrumbsIntegration()wrapper functionparseStackTrace()function andIStackFrameinterfaceBetter separation of concerns:
Enhanced documentation:
Exports from error-capture.tsx:
IErrorData- Generic error data interfaceIErrorContext- Context data (breadcrumbs, tags, session, URL)IBreadcrumb- Breadcrumb structureinit()- Initialize error capture with callbackcaptureException()- Capture and report errorstrackNavigation()- Track navigation breadcrumbsformatErrorMessage()- Format error messages genericallyformatStackTrace()- Clean up stack traces genericallyBenefits
Testing
✅ All 23 tracking tests pass
✅ TypeScript compilation successful
✅ Production build successful
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.