[HEIHEI-147] add TraceManagerDebugger #30
Conversation
There was a problem hiding this comment.
Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/v3/matchSpan.ts:307
- Merging definitions via Object.assign may lead to unintended overwrites if multiple matchers provide conflicting fromDefinition properties. Consider using a deep merge or clearly documenting the merge behavior to avoid potential conflicts.
if (matcher.fromDefinition) {
src/v3/Tracer.ts:205
- The updated error message may confuse users about the trace's state by switching from an 'interrupt' to an 'access' context. Please verify that the message accurately reflects the intended behavior and context.
new Error(`Trying to access '${this.definition.name}' trace, however the started trace (${trace.sourceDefinition.name}) has a different definition`)
xnanodax
force-pushed
the
report-error-fns
branch
from
fac558b to
e7a6ee8
Compare
|
note: i ended removing 6e83aba from |
bbrzoska
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR improves trace management functionality by introducing the TraceManagerDebugger along with updates to trace state transitions and event reporting. Key changes include refactoring of the trace recording utilities and updates to test cases and component stories to incorporate the new debugger and console logger.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/v3/requiredSpanWithErrorStatus.ts | Added a clarifying comment for special span matching on error status. |
| src/v3/recordingComputeUtils.ts | Updated parameter naming and switched from a ReadonlySet to a readonly array for recorded items. |
| src/v3/recordingComputeUtils.test.ts | Updated test inputs to match the new state parameter naming conventions. |
| src/v3/matchSpan.ts | Revised handling of the “oneOf” matcher definitions to combine sub-matchers using new logic. |
| src/v3/debugTypes.ts | Introduced new type definitions for debugging events. |
| src/v3/convertToRum.test.ts | Adjusted test parameters to reflect recent changes in state transition properties. |
| src/v3/TraceManagerWithDraft.test.ts | Modified test expectations to account for the updated reporting behavior on interrupt events. |
| src/v3/TraceManager.ts | Added event subjects and corresponding “when” overloads for forwarding trace events. |
| src/v3/Trace.ts | Extended the trace implementation with event emission for debugging, property resets, and refactored API. |
| src/stories/mockComponentsv3/* | Updated stories and component traces to integrate the new TraceManagerDebugger and ConsoleTraceLogger. |
| src/main.ts | Exported the new ConsoleTraceLogger and TraceManagerDebugger for broader consumption. |
| .storybook/main.ts | Removed unused Storybook plugin import. |
Files not reviewed (1)
- package.json: Language not supported
| this.processedPerformanceEntries = new WeakMap() | ||
| // @ts-expect-error memory cleanup force override the otherwise readonly property |
There was a problem hiding this comment.
Using @ts-expect-error to override a readonly property can mask potential issues. Consider refactoring to make recordedItemsByLabel writable or providing a dedicated cleanup method that avoids type overrides.
Copilot uses AI. Check for mistakes.
| return { | ||
| transitionToState: 'complete', | ||
| transitionToState: 'interrupted', |
There was a problem hiding this comment.
why did we transition to complete in the first place? cant remember!
There was a problem hiding this comment.
I think it was an unintentional mistake
| finalTransition?: FinalTransition<RelationSchemasT> | ||
| } | ||
|
|
||
| const styles = { |
There was a problem hiding this comment.
hm, would be able to move styles into another file?
There was a problem hiding this comment.
keeping them here for now cause copilot iteration is simpler
…k’s render‐time fix for Warning: Cannot update a component (`TraceManagerDebugger`) while rendering a different component (`TicketView`). To locate the bad setState() call inside `TicketView`.
|
🎉 This PR is included in version 2.4.0-next.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Screen.Recording.2025-04-04.at.01.00.52.mov