Skip to content

feat(feedback): show issue details' TraceDataSection in feedback details#76372

Closed
aliu39 wants to merge 8 commits into
masterfrom
aliu/feedback-trace
Closed

feat(feedback): show issue details' TraceDataSection in feedback details#76372
aliu39 wants to merge 8 commits into
masterfrom
aliu/feedback-trace

Conversation

@aliu39

@aliu39 aliu39 commented Aug 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #68481

For the screenshots, I used dev-ui to override the feedback's trace ID. Before merging, we need to test this by finding/creating some real-world, prod examples.

  1. Trace w/26 issues:

trace-ex1

You can try interacting with the same timeline in this issue.

  1. Trace w/1 other issue:

trace-ex2

  1. If missing trace, or trace has 0 other issues, the section is not rendered at all

@aliu39 aliu39 requested a review from a team as a code owner August 16, 2024 23:56
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 16, 2024
michellewzhang
michellewzhang reviewed Aug 26, 2024
View reviewed changes
Comment thread static/app/components/feedback/feedbackItem/feedbackItem.tsx
);
}

function TraceDataSection({

@michellewzhang michellewzhang Aug 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we move this to a new file? similar to how the other sections are structured

@aliu39 aliu39 Aug 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hm I feel like it's small enough to keep here, it's basically a wrapper around Issue's TraceDataSection

michellewzhang
michellewzhang approved these changes Aug 26, 2024
View reviewed changes

@michellewzhang michellewzhang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

before we merge, could we find some real examples to test this component on? i bet @bruno-garcia could dig up some from nugettrends or elsewhere

@aliu39

aliu39 commented Aug 26, 2024

Copy link
Copy Markdown
Member Author

before we merge, could we find some real examples to test this component on? i bet @bruno-garcia could dig up some from nugettrends or elsewhere

Definitely! To be clear, we need 2+ unique issues in the same trace as the feedback. Excluding the feedback itself

@aliu39

aliu39 commented Aug 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Here's a good prod example: https://sentry.dev.getsentry.net:7999/feedback/?feedbackSlug=javascript%3A5764397098&project=11276
When first created this had a timeline, but since the errors were the same, they were grouped to 1 issue.

https://sentry.dev.getsentry.net:7999/feedback/?feedbackSlug=javascript%3A5736701211
Example with both a trace-related issue and a crash report. Note the trace link (/performance) is different from the crash report link (/issues/:groupId)

Still need a timeline example

@michellewzhang

michellewzhang commented Aug 29, 2024

Copy link
Copy Markdown
Contributor

looks good! there's some timeline examples on nugettrends that look good to me

@aliu39 aliu39 requested review from a team, bruno-garcia and jas-kas August 29, 2024 20:56
@codecov

codecov Bot commented Aug 29, 2024

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
7717 1 7716 0
View the top 1 failed tests by shortest run time 
trace view keyboard navigation arrowdown on last node jumps to start trace view keyboard navigation arrowdown on last node jumps to start
Stack Traces | 2.63s run time 
Error: Unable to find an element with the text: /transaction-op-0/i. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style
...
    at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
    at keyboardNavigationTestSetup (.../performance/newTraceDetails/trace.spec.tsx:273:52)
    at Object.<anonymous> (.../performance/newTraceDetails/trace.spec.tsx:755:17)
    at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:540:9)
    at processTimers (node:internal/timers:514:7)
    at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@bruno-garcia

bruno-garcia commented Sep 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Here's a feedback with a trace:

Feedback page
Feedback in issue detail
The crash report event linked to the feedback for reference

@aliu39 aliu39 mentioned this pull request Sep 3, 2024
Merged
@aliu39

aliu39 commented Sep 3, 2024

Copy link
Copy Markdown
Member Author

Reopened at #76858

@aliu39 aliu39 closed this Sep 3, 2024
aliu39 added a commit that referenced this pull request Sep 3, 2024
@aliu39
feat(feedback): add trace section to details, with analytics and FF (#…
c8173e9 
…76858)

Branched from #76372 (at first I
thought this branch would be for analytics only, but now it replaces
this PR)

Depends on #76867
@aliu39 aliu39 deleted the aliu/feedback-trace branch September 3, 2024 22:26
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bruno-garcia bruno-garcia Awaiting requested review from bruno-garcia

@jas-kas jas-kas Awaiting requested review from jas-kas

1 more reviewer

@michellewzhang michellewzhang michellewzhang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[User Feedback] - Issue Trace Timeline

3 participants

@aliu39 @michellewzhang @bruno-garcia