feat: show exit calls break up on hover on exit calls cell in trace list #756

itssharmasandeep · 2021-04-08T10:48:48Z

Description

feat: show exit calls break up on hover on exit calls cell in trace list

Testing

Local testing is done

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Working example for 0 exit calls

Working example for more than 0 exit calls

codecov · 2021-04-08T10:52:20Z

Codecov Report

Merging #756 (b0c8c46) into main (4776e67) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #756   +/-   ##
=======================================
  Coverage   85.37%   85.38%           
=======================================
  Files         788      789    +1     
  Lines       16132    16143   +11     
  Branches     2060     2060           
=======================================
+ Hits        13772    13783   +11     
  Misses       2329     2329           
  Partials       31       31

Impacted Files	Coverage Δ
...apis/api-detail/traces/api-trace-list.dashboard.ts	`100.00% <ø> (ø)`
...vice-detail/traces/service-trace-list.dashboard.ts	`100.00% <ø> (ø)`
...ers/traces/traces-graphql-query-handler.service.ts	`100.00% <100.00%> (ø)`
...-calls/exit-calls-table-cell-renderer.component.ts	`100.00% <100.00%> (ø)`
.../table/observability-table-cell-renderer.module.ts	`100.00% <100.00%> (ø)`

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 4776e67...b0c8c46. Read the comment docs.

aaron-steinfeld

Could we see a screenshot of this in action?

...c/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.scss

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts

aaron-steinfeld · 2021-04-09T12:33:21Z

...hared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.test.ts


-    expect(spectator.component.getMaxShowAPICalleeNameCount(value)).toMatchObject(value);
-    expect(spectator.component.totalCountOfDifferentAPICallee).toBe(2);
+    expect(spectator.component.apiCalleeNameCount).toMatchObject([


This test has value, but what would be even better would be to verify that this data renders - for example by querying for the rendered spans and checking their content (e.g. expect(spectator.queryAll('.api-callee-name')[0]).toContainText('key1');)

That checks both how the data is processed but also that the template is correctly binding what we expect.

Okay, this change I can make.

Nevermind, discussed offline - because this is a template that's not rendered in the scope of this component, it's ugly to test and not worth it.

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts

arjunlalb · 2021-04-14T09:02:09Z

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts

+      <span class="exit-calls-count">{{ this.apiExitCalls }}</span>
+
+      <ng-template #exitCallsTooltip>
+        <ng-container *ngIf="this.apiExitCalls > 0">


nit: This is an if-else use case. You can clean this up like below. There are many examples in the codebase.

<ng-container *ngIf="this.apiExitCalls > 0; else noExitCalls"> Content goes here </ng-container> <ng-template #noExitCalls></ng-template>

arjunlalb

LGTM

aaron-steinfeld · 2021-04-14T14:52:07Z

lgtm other than existing comment on test

github-actions · 2021-04-14T15:18:13Z

Unit Test Results

    4 files ±0 248 suites +1 16m 4s ⏱️ + 2m 2s
888 tests +1 888 ✔️ +1 0 💤 ±0 0 ❌ ±0
894 runs +1 894 ✔️ +1 0 💤 ±0 0 ❌ ±0

Results for commit 4325a3c. ± Comparison against base commit 4776e67.

anandtiwary · 2021-04-14T15:12:03Z

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts

+    @Inject(TABLE_ROW_DATA) rowData: Trace
+  ) {
+    super(columnConfig, index, parser, cellData, rowData);
+    const apiCalleeNameCount: string[][] = Object.entries(cellData.value[1]);


This name is confusing to me mainly because of 'Count' suffix. So apiCalleeNameCount is a dictionary?

can we call it apiCalleeNameEntires instead?

Is [string, string][] a better type?

totalCountOfDifferentApiCallee -> uniqueApiCallee

Line 71. Why do we need to use slice here?

Agree naming is confusing/type suggestions. Line 71 is meant to limit the amount of pairs to show in the tooltip, but again naming would help clarify (maxShowApiCalleeNameCount as a const value could be MAX_API_CALLEE_TO_SHOW, for example).

Let's also rename maxShowApiCalleeNameCount to MAX_API_CALLEE_TO_SHOW

anandtiwary · 2021-04-14T15:12:59Z

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts

+import { Trace } from '@hypertrace/distributed-tracing';
+import { ObservabilityTableCellType } from '../../observability-table-cell-type';
+
+interface CellData {


{ type: 'composite-specification', specifications: [ { type: 'attribute-specification', attribute: 'apiExitCalls' }, { type: 'attribute-specification', attribute: 'apiCalleeNameCount' } ], 'order-by': 'apiExitCalls' }, }

Wouldn't the cell data be an array of [apiExitCalls, apiCalleeNameCount] ? Trying to understand how this object structure is coming with units and value info

the query handlers append units on to everything so instead of that array, it's wrapped in an object with units like {units?: string; value: [apiExitCalls, apiCalleeNameCount]}

Did this change recently?

I remember using composite spec where I have used value as array directly(let me find it). And regardless, imo the specification should reflect this object structure.

export interface CompositeSpecification extends Specification { extractFromServerData(resultContainer: Dictionary<unknown>): unknown[]; }

Possibly, table is calling parser's parseValue is called after stripping units.

This does seem a bit suspicious - the wrapping object should only be there if units are defined, which they shouldn't be for a composite spec.

feat: show exit calls break up on hover on exit calls cell in trace list

c13757a

itssharmasandeep requested a review from a team as a code owner April 8, 2021 10:48

This comment has been minimized.

Sign in to view

Merge branch 'main' into exit-calls

040f804

aaron-steinfeld reviewed Apr 8, 2021

View reviewed changes

arjunlalb reviewed Apr 9, 2021

View reviewed changes

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts Outdated Show resolved Hide resolved

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts Outdated Show resolved Hide resolved

fix: addressing pull request comments

b92bbea

This comment has been minimized.

Sign in to view

fix: lint errors

d56cb57

This comment has been minimized.

Sign in to view

fix: addressing review commnet

1f2f9d1

This comment has been minimized.

Sign in to view

Merge branch 'main' into exit-calls

5d5126d

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 9, 2021

View reviewed changes

anandtiwary reviewed Apr 9, 2021

View reviewed changes

...src/shared/components/table/data-cell/exit-calls/exit-calls-table-cell-renderer.component.ts Outdated Show resolved Hide resolved

itssharmasandeep added 3 commits April 14, 2021 13:01

chore: using composite specification

1c394d6

fix: lint errors and tests

0584eb7

fix: lint errors

541751a

This comment has been minimized.

Sign in to view

Merge branch 'main' into exit-calls

b0c8c46

This comment has been minimized.

Sign in to view

arjunlalb reviewed Apr 14, 2021

View reviewed changes

aaron-steinfeld approved these changes Apr 14, 2021

View reviewed changes

itssharmasandeep merged commit 4325a3c into main Apr 14, 2021

itssharmasandeep deleted the exit-calls branch April 14, 2021 15:13

anandtiwary reviewed Apr 14, 2021

View reviewed changes

feat: show exit calls break up on hover on exit calls cell in trace list #756

feat: show exit calls break up on hover on exit calls cell in trace list #756

Uh oh!

Conversation

itssharmasandeep commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

codecov bot commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

arjunlalb Apr 14, 2021 • 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!

arjunlalb left a comment

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld commented Apr 14, 2021

Uh oh!

github-actions bot commented Apr 14, 2021

Unit Test Results

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!

itssharmasandeep commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

arjunlalb Apr 14, 2021 •

edited

Loading