feat(apm): Initial transactions view #13920

dashed · 2019-07-08T21:53:32Z

WORK IN PROGRESS

PR dependencies

NOTE: The spans interface implementation from feat(apm) Add spans interface to events #13974 was pulled into this PR. It'll be removed once that PR is merged and this PR is rebased against master.
Depends on feat(apm) Add spans interface to events #13974 (Spans Interface implementation on Django side)
Depends on fix: Remove zoom css property. #14002 (remove zoom css) for proper minimap functionality.
Depends on feat(apm) Expose transaction start & end timestamp #14007
Depends on feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) #14040
waiting on feat: Update to JS SDK 5.6.0-beta.1 + 0.10.2 sentry-python #14116
waiting on https://github.com/getsentry/getsentry/pull/3065

TODO (top-level)

spans interface - moved to feat(apm): Update props to address proptype warnings for new transaction attributes (SEN-800) #14040
event transaction view
implement trace view
~~adjust tags for spans view~~ EDIT: moved to feat(eventsv2) Add basic transaction list #14103
simplify lodash imports
come up with better names for things like types
remove stray console.logs
open modal on transactionSlug
vertical markers for duration

TODO (trace view)

There are 2 versions of the trace view that can be explored.

TODO (minimap)

Deferred to a follow-up PR

This follow-up PR will cover https://getsentry.atlassian.net/browse/SEN-866

acceptance test: trace view
acceptance test: empty transactions events list
acceptance test: non-empty transactions events list
drag view window (without changing its window size)
select window size by dragging onto the minimap
mouse guide cursor should display current duration + timestamp
mouse guide cursor should display a guide cursor in the trace view
russian doll-ing of spans for collapsed span trees (maybe)

Closes SEN-808
Closes SEN-846

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

src/sentry/static/sentry/app/components/events/interfaces/minimap.tsx

src/sentry/static/sentry/app/components/events/interfaces/spans.tsx

markstory · 2019-07-15T15:11:57Z

src/sentry/static/sentry/app/components/events/interfaces/spans.tsx

+
+    const parsedTrace = this.parseTrace();
+
+    // TODO: ideally this should be provided


Would you want this from the API? The API could ensure that all spans are ordered by start timestamp.

Yep :D

I saw #14007

src/sentry/static/sentry/app/components/events/interfaces/spans.tsx

codecov · 2019-07-20T05:11:50Z

Codecov Report

Merging #13920 into master will increase coverage by 3.95%.
The diff coverage is 21.71%.

@@            Coverage Diff             @@
##           master   #13920      +/-   ##
==========================================
+ Coverage    82.7%   86.66%   +3.95%     
==========================================
  Files        3221     3230       +9     
  Lines      140352   140847     +495     
  Branches     4953     5009      +56     
==========================================
+ Hits       116077   122059    +5982     
+ Misses      22887    17398    -5489     
- Partials     1388     1390       +2

Impacted Files	Coverage Δ
src/sentry/static/sentry/app/sentryTypes.jsx	`100% <ø> (ø)`	⬆️
...static/sentry/app/components/eventOrGroupTitle.jsx	`100% <ø> (ø)`	⬆️
...atic/sentry/app/components/events/eventEntries.jsx	`40% <ø> (-4.45%)`	⬇️
...atic/sentry/app/components/events/groupingInfo.jsx	`21.66% <ø> (ø)`	⬆️
...tatic/sentry/app/components/eventOrGroupHeader.jsx	`63.63% <ø> (ø)`	⬆️
...tic/sentry/app/views/organizationEventsV2/data.jsx	`47.91% <0%> (-5.58%)`	⬇️
...y/app/components/events/interfaces/spans/index.tsx	`0% <0%> (ø)`
...omponents/events/interfaces/spans/drag_manager.tsx	`12% <12%> (ø)`
...nents/events/interfaces/spans/transaction_view.tsx	`18.18% <18.18%> (ø)`
...p/components/events/interfaces/spans/span_tree.tsx	`20.39% <20.39%> (ø)`
... and 340 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b141ab...a2b3050. Read the comment docs.

src/sentry/static/sentry/app/components/events/interfaces/spans/drag_manager.tsx

src/sentry/static/sentry/app/components/events/interfaces/spans/dragManager.tsx

src/sentry/static/sentry/app/components/events/interfaces/spans/minimap.tsx

src/sentry/static/sentry/app/components/events/interfaces/spans/spanDetail.tsx

src/sentry/static/sentry/app/components/events/interfaces/spans/span_tree.tsx

markstory

Mashing approve on this as we need this to unblock other work around the APM-ui and tests can be added once the implementation/design settle down a bit.

billyvg · 2019-07-25T16:12:43Z

src/sentry/static/sentry/app/components/events/interfaces/spans/dragManager.tsx

+      return;
+    }
+
+    const rect = rectOfContent(this.props.interactiveLayerRef.current!);


what does the ! do at the end?

It tells the TS compiler that current will is not going to be null/undefined

The ! operator is useful in cases when TS isn't smart enough to know that this.props.interactiveLayerRef.current has been checked to not be null earlier.

more info here: https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#non-null-assertion-operator

This operator will definitely be useful during the TS migration.

billyvg · 2019-07-25T16:13:51Z

src/sentry/static/sentry/app/components/events/interfaces/spans/dragManager.tsx

+
+    // remove listeners that were attached in onDragStart
+
+    window.removeEventListener('mousemove', this.onDragMove);


Is it possible this component unmounts before a dragend event happens?

Very good point 👍

I'm going to consolidate the clean up steps into a function (e.g. restoring the body style) on the next iteration of the APM frontend implementation.

billyvg · 2019-07-25T16:16:58Z

src/sentry/static/sentry/app/components/events/interfaces/spans/minimap.tsx

+
+  render() {
+    return (
+      <React.Fragment>


Is this needed?

billyvg · 2019-07-25T16:19:07Z

src/sentry/static/sentry/app/components/events/interfaces/spans/minimap.tsx

+              });
+            }}
+          >
+            <InteractiveLayer style={{overflow: 'visible'}}>


Can we move the inline style into the styled component?

billyvg · 2019-07-25T16:20:34Z

src/sentry/static/sentry/app/components/events/interfaces/spans/minimap.tsx

+    );
+  };
+
+  renderViewHandles = ({


It might make Minimap a bit easier to read if some of these render methods were components.

billyvg · 2019-07-25T16:30:29Z

src/sentry/static/sentry/app/components/events/interfaces/spans/spanTree.tsx

+      <React.Fragment>
+        <SpanRow
+          data-span-hidden={isVisible ? 'false' : 'true'}
+          style={{


Would prefer this to be handled in the SC

src/sentry/static/sentry/app/components/events/interfaces/spans/spanTree.tsx

billyvg · 2019-07-25T16:33:14Z

src/sentry/static/sentry/app/components/events/interfaces/spans/spanTree.tsx

+  transition: background-color 0.15s ease-in-out;
+
+  &:last-child {
+    & > [data-component='span-detail'] {


We should avoid using data attributes for presentation like this

billyvg · 2019-07-25T16:36:58Z

src/sentry/static/sentry/app/components/events/interfaces/spans/spanTree.tsx

+  &:hover {
+    background-color: rgba(189, 180, 199, 0.1);
+
+    & > [data-span='true'] {


e.g. for this we could have in SpanBar

${SpanRow}:hover & { transition: border-color 0.15s ease-in-out; border: 1px solid rgba(0, 0, 0, 0.1); }

Otherwise we have another set of attributes to keep track of (and there's no build time checks for data attribute refereces)

billyvg · 2019-07-25T16:40:32Z

src/sentry/static/sentry/app/components/events/interfaces/spans/transactionView.tsx

+
+    // sort span children by their start timestamps in ascending order
+
+    forEach(reduced.lookup, spanChildren => {


nit, can we use Object.values().forEach() instead of lodash?

Yep will do 😄

billyvg · 2019-07-25T16:42:54Z

sorry, late to the party @dashed

dashed · 2019-07-27T06:36:26Z

@billyvg Apologizes, I just these your feedback comments just now.

I'm going to note all of them down for the next iteration of the APM UI.

Due to the nature of the new designs, I will be throwing away some of the implementation from this PR.

dashed added the WIP label Jul 8, 2019

dashed self-assigned this Jul 8, 2019

markstory reviewed Jul 9, 2019

View reviewed changes

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

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

dashed force-pushed the transactions-view branch 4 times, most recently from 98faa8b to 607e0e1 Compare July 12, 2019 21:02

dashed mentioned this pull request Jul 14, 2019

fix: Remove zoom css property. #14002

Merged

dashed force-pushed the transactions-view branch from b9b15fa to f6d2081 Compare July 15, 2019 04:54

markstory reviewed Jul 15, 2019

View reviewed changes

dashed force-pushed the transactions-view branch 5 times, most recently from 30c6aa2 to 446f54e Compare July 18, 2019 22:01

dashed changed the title ~~feat(ui): Initial transactions view~~ feat(apm): Initial transactions view Jul 19, 2019

dashed changed the title ~~feat(apm): Initial transactions view~~ feat(aom): Initial transactions view Jul 19, 2019

dashed changed the title ~~feat(aom): Initial transactions view~~ feat(apm): Initial transactions view Jul 19, 2019

dashed force-pushed the transactions-view branch from 0f3b321 to f593680 Compare July 19, 2019 16:23

dashed force-pushed the transactions-view branch from c16ae14 to 006bd9d Compare July 22, 2019 16:15

markstory reviewed Jul 22, 2019

View reviewed changes

src/sentry/static/sentry/app/components/events/interfaces/spans/drag_manager.tsx Outdated Show resolved Hide resolved

dashed force-pushed the transactions-view branch 4 times, most recently from ef17136 to 581df13 Compare July 23, 2019 15:00

markstory reviewed Jul 23, 2019

View reviewed changes

dashed force-pushed the transactions-view branch 3 times, most recently from 8e552bf to 727eac9 Compare July 23, 2019 23:29

dashed added 19 commits July 25, 2019 10:15

TraceContextType

b879451

Rect type

b36da9c

throw errors on unexpected ViewHandleType type

bb662a9

root_rect => rootRect

31f8941

useless use of React.Fragment. kinda like useless use of cat.

fff5913

resize_canvas => resizeCanvas

86abd80

camels.

36e2976

rename

21379b4

move styles

aa97eca

decrease lodash import scope

80289e5

camels

c78ac7a

display # of hidden spans below

a70298d

init time axis

2e44967

parse trace earlier

8d5ee44

things

4b56b11

more timeaxis

de3f7c4

improve ticks

6f84d95

nit

f24a662

use startTimestamp and endTimestamp

ce5b6ee

dashed force-pushed the transactions-view branch from 38e4cff to ce5b6ee Compare July 25, 2019 14:33

overflow visible

9fa70c0

markstory approved these changes Jul 25, 2019

View reviewed changes

lint fix

a0e6f5c

dashed merged commit 9b4a214 into master Jul 25, 2019

dashed deleted the transactions-view branch July 25, 2019 16:10

billyvg reviewed Jul 25, 2019

View reviewed changes

dashed mentioned this pull request Jul 31, 2019

feat(apm): Transactions view v1 #14225

Merged

19 tasks

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


		const parsedTrace = this.parseTrace();

		// TODO: ideally this should be provided


		// remove listeners that were attached in onDragStart

		window.removeEventListener('mousemove', this.onDragMove);


		// sort span children by their start timestamps in ascending order

		forEach(reduced.lookup, spanChildren => {

Uh oh!

feat(apm): Initial transactions view #13920

feat(apm): Initial transactions view #13920

Uh oh!

Conversation

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

Uh oh!

PR dependencies

TODO (top-level)

TODO (trace view)

TODO (minimap)

Deferred to a follow-up PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markstory 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billyvg commented Jul 25, 2019

Uh oh!

dashed commented Jul 27, 2019

Uh oh!

Uh oh!

dashed commented Jul 8, 2019 •

edited

Loading

codecov bot commented Jul 20, 2019 •

edited

Loading