feat(apm): Transactions view v1 #14225

dashed · 2019-07-31T12:20:23Z

Continuation of #13920

NOTES

this PR revises the transaction view to be a two column split-view as per Crissy's updated designs (SEN-866)
transactions with no descendant spans are now properly displayed (SEN-873)
for transaction events, the events interfaces within the eventsv2 modal are replaced with only the transaction view component
design is still influx; don't be surprised that there are no tests.

TODO

Deferred to a followup PR

span bar error visual cue for top-level span (SDK team says this metadata will be added)
russian doll-ing of spans for collapsed span trees (maybe)

Closes SEN-866
Closes SEN-873

src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx

lynnagara · 2019-08-02T22:42:41Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/span.tsx

would showDetail be a bit clearer / more in line with showSpanTree?

@lynnagara I don't quite follow; are you suggesting to rename showDetail to showSpanTree? 🤔

If so, it would be the wrong name since span-descendants aren't displayed within the span detail component.

Haha no sorry, I thought showDetail might be better than displayDetail, since we use the show- prefix with the span tree stuff

Done in 3dac2a6

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/span.tsx

dashed · 2019-08-06T16:09:18Z

This PR is somewhat ready for review. The last few remaining tasks entails reorganizing, refactoring, and splitting up files.

lynnagara

I had a very quick glance but it's quite a large PR. If time permits, it could be nice to split this into smaller, easier to review chunks with tests and more documentation/comments. It'd also be nice to have a Percy test so it's easier for reviewers to spot the visual changes in these new screens.

lynnagara · 2019-08-06T19:13:04Z

...entry/static/sentry/app/views/organizationEventsV2/transactionView/dividerHandlerManager.tsx

data-test-id prefix should only be used in tests and was mentioned previously that it might one day be stripped as part of the build process

Also, why do we need to use querySelectorAll to select the element? Can't we pass the refs or something like that?

@lynnagara I refactored to make use of refs here 5a8bb7f

...entry/static/sentry/app/views/organizationEventsV2/transactionView/dividerHandlerManager.tsx

lynnagara · 2019-08-06T19:38:41Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/utils.tsx

Would be nice to have some docs and tests on some of these functions

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/utils.tsx

package.json

dashed · 2019-08-07T02:41:54Z

@lynnagara Kudos for taking a look over this PR 👏 ; I really appreciate it.

I agree that this PR has gotten longer than I had anticipated. 😞 I'll see if I can split this PR into two parts.

...entry/static/sentry/app/views/organizationEventsV2/transactionView/dividerHandlerManager.tsx

markstory · 2019-08-07T15:32:22Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/dragManager.tsx

-    document.body.style.userSelect = 'none';
+    this.previousUserSelect = setBodyUserSelect({
+      userSelect: 'none',
+      MozUserSelect: 'none',


Why is M capitalized for Moz but not ms?

@markstory Good question. I had to look this up to double check.

I got this from https://stackoverflow.com/a/31998812

Vendor prefixes other than ms should begin with a capital letter.

from: https://reactjs.org/docs/dom-elements.html#style

markstory · 2019-08-07T15:37:23Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/minimap.tsx

+      }
+      default: {
+        const _exhaustiveCheck: never = bounds;
+        return _exhaustiveCheck;


Should this be an exception?

@markstory it doesn't need to be.

I didn't add one because this file is consumed only by other typescript files. So we defer to the TS engine to ensure we've added all necessary the cases for the switch statement. This relies on --strictNullChecks being enabled.

AFAIK, there are two documented ways to do exhaustive checks for switch-case: https://www.typescriptlang.org/docs/handbook/advanced-types.html#exhaustiveness-checking

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/minimap.tsx

markstory · 2019-08-07T15:41:04Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/minimap.tsx

+  position: sticky;
+  left: 0;
+  top: 0;
+  z-index: 99999999999;


Could we use theme.zIndex here?

I've decided to collate the z-indices locally 3fe6709

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/spanBar.tsx

markstory · 2019-08-07T15:50:29Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/spanBar.tsx

+
+  margin-right: 8px;
+
+  z-index: 99999;


Another place we should use theme.zIndex.

I've decided to collate the z-indices locally 3fe6709

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/utils.tsx

lynnagara · 2019-08-07T20:36:58Z

Is netlify preview working? Would be useful for this one

lynnagara

Can we share any renderSpan code between spanTree vs minimap? Looks very similar at a glance.

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/traceView.tsx

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/spanTree.tsx

lynnagara · 2019-08-07T20:34:07Z

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/spanTree.tsx

+        const results = this.renderSpan({
+          spanNumber: acc.nextSpanNumber,
+          treeDepth: treeDepth + 1,
+          numOfHiddenSpansAbove: acc.numOfHiddenSpansAbove,


What is a "hidden" span?

Hidden spans are spans that are out of view due to the chosen view window.

This is useful for tall span trees so we can collapse unnecessary real-estate space in terms of height for better UX.

dashed · 2019-08-07T22:24:14Z

@lynnagara I'm unsure how to access events v2 from sentry-test org in netlify. It doesn't seem possible.

Regarding renderSpan() in both the minimap and spanTree implementation, I'm aware of their similarities. I prefer to duplicate some parts rather than attempting to abstract the function.

Thanks for the feedback 👍

…ctionView/dividerHandlerManager.tsx Co-Authored-By: Mark Story <[email protected]>

dashed added the WIP label Jul 31, 2019

dashed self-assigned this Jul 31, 2019

dashed force-pushed the apm/transactions-view-v1 branch from c48f2fe to c1b3039 Compare July 31, 2019 12:24

markstory reviewed Jul 31, 2019

View reviewed changes

src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx Outdated Show resolved Hide resolved

dashed force-pushed the apm/transactions-view-v1 branch 4 times, most recently from e08d532 to b8815d9 Compare August 2, 2019 16:00

lynnagara reviewed Aug 2, 2019

View reviewed changes

src/sentry/static/sentry/app/views/organizationEventsV2/eventModalContent.jsx Outdated Show resolved Hide resolved

lynnagara reviewed Aug 2, 2019

View reviewed changes

src/sentry/static/sentry/app/views/organizationEventsV2/transactionView/span.tsx Outdated Show resolved Hide resolved

dashed force-pushed the apm/transactions-view-v1 branch 4 times, most recently from b9476c5 to 6795cc8 Compare August 6, 2019 15:46

dashed marked this pull request as ready for review August 6, 2019 16:07

dashed requested a review from markstory August 6, 2019 16:07

lynnagara reviewed Aug 6, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

dashed force-pushed the apm/transactions-view-v1 branch from 3f47d07 to 846c4f5 Compare August 7, 2019 03:17

markstory reviewed Aug 7, 2019

View reviewed changes

dashed requested review from lynnagara and markstory August 7, 2019 20:23

lynnagara reviewed Aug 7, 2019

View reviewed changes

dashed force-pushed the apm/transactions-view-v1 branch from 17020b4 to d6322d9 Compare August 7, 2019 22:19

dashed removed the WIP label Aug 8, 2019

dashed and others added 26 commits August 8, 2019 14:58

refactor spanBar from spanGroup

732b2ad

clean up

d640c97

remove fragment

dcfa94e

address browser zoom levels

007b61e

prefer refs over data-test-ids

a01de3c

min-width on span bars

bc81d90

VIEW_HANDLE_HEIGHT magic constant

dc36cc1

bug fix

52488b4

Update src/sentry/static/sentry/app/views/organizationEventsV2/transa…

ccd44ab

…ctionView/dividerHandlerManager.tsx Co-Authored-By: Mark Story <[email protected]>

properly clean up listeners on unmount

e1920f0

docs

4177115

docs

2ca001c

docs

14febef

🚲

084c283

performance.

3dc68fd

change name from displayDetail to showDetail

43a32a9

modern browsers only

c5a7038

un-used id

f11d77d

collate z-index

530ddd1

docs

46bfdb6

lint fix

7704cca

derive information from spans

f50bd3a

rename lookup to childSpans

42c58bc

refactor

9b724ba

use spanNumber to pick span colours

1bd38e2

adjust traceViewRef

d3b87d2

dashed force-pushed the apm/transactions-view-v1 branch from 9b973bc to d3b87d2 Compare August 8, 2019 18:59

dashed merged commit c0fa8e0 into master Aug 8, 2019

dashed deleted the apm/transactions-view-v1 branch August 8, 2019 19:38

github-actions bot locked and limited conversation to collaborators Dec 20, 2020

Uh oh!

feat(apm): Transactions view v1 #14225

feat(apm): Transactions view v1 #14225

Uh oh!

Conversation

dashed commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NOTES

TODO

Deferred to a followup PR

Uh oh!

Uh oh!

Uh oh!

lynnagara Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dashed commented Aug 6, 2019

Uh oh!

lynnagara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dashed commented Aug 7, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lynnagara commented Aug 7, 2019

Uh oh!

lynnagara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dashed commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dashed commented Jul 31, 2019 •

edited

Loading

lynnagara Aug 2, 2019 •

edited

Loading

dashed commented Aug 7, 2019 •

edited

Loading