Trace drilldown to explorer screen #1098

jyothishjose6190 · 2021-08-31T09:56:08Z

Screen.Recording.2021-09-24.at.3.13.51.PM.mov

Enables the user to click on a trace ID and view the explorer screen with filters applied

on a feature: User Can click on trace ID in trace view, full trace view and it will take the user to the explorer screen with the trace ID filter applied

Testing

All tests passed

Checklist:

[x ] My changes generate no new warnings

codecov · 2021-08-31T10:00:27Z

Codecov Report

Merging #1098 (484cc8f) into main (594a8db) will increase coverage by 0.09%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
+ Coverage   85.08%   85.18%   +0.09%     
==========================================
  Files         825      825              
  Lines       17066    17082      +16     
  Branches     2216     2216              
==========================================
+ Hits        14521    14551      +30     
+ Misses       2514     2500      -14     
  Partials       31       31

Impacted Files	Coverage Δ
...s/waterfall/waterfall-widget-renderer.component.ts	`47.50% <83.33%> (+3.05%)`	⬆️
...pi-trace-detail/api-trace-detail.page.component.ts	`100.00% <100.00%> (+61.53%)`	⬆️
...s/api-trace-detail/api-trace-detail.page.module.ts	`100.00% <100.00%> (ø)`
.../pages/trace-detail/trace-detail.page.component.ts	`100.00% <100.00%> (+58.33%)`	⬆️
...src/pages/trace-detail/trace-detail.page.module.ts	`100.00% <100.00%> (ø)`
...board/widgets/waterfall/waterfall-widget.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 594a8db...484cc8f. Read the comment docs.

aaron-steinfeld

only commented on one, but they apply in both places

aaron-steinfeld · 2021-08-31T12:05:47Z

projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts

+          <ng-container
+            *ngIf="this.getExploreNavigationParams(traceDetails.traceId) | async as exploreNavigationParams"
+          >
+            <ht-link class="link" [paramsOrUrl]="exploreNavigationParams">


Could you add a screenshot of what this looks like after the change? Specifically, I'm looking for what this looks like vs what it would look like to use ExploreFilterLinkComponent instead of htLink directly.

got it, thanks - I think for consistency, it would make since to use the filter link here. It also gives a visual indicator, without requiring a hover, that it's actionable.

Something like:

<ht-explore-filter-link [paramsOrUrl]="this.getExploreNavigationParams(traceDetails.traceId) | async" <ht-summary-value class="summary-value" icon="${IconType.TraceId}" label="Trace ID" [value]="traceDetails.traceId"> </ht-summary-value> </ht-explore-filter-link>

Yep, I thought we recommended ht-explore-filter-link only. It is same as ht-link but it brings in an icon at the end that indicates the user what type of link it is.

aaron-steinfeld · 2021-08-31T12:08:15Z

projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts

-          ></ht-summary-value>
+
+          <ng-container
+            *ngIf="this.getExploreNavigationParams(traceDetails.traceId) | async as exploreNavigationParams"


1 nit, 1 major here:
nit - the ngIf can move down onto the link, the extra container is unnecessary.
major - calling a function to return an observable will return a new observable each time - so every cycle will be treated as a change from the async pipe's perspective. Instead, you can either assign the observable to a variable and reference that here, or use htMemoize pipe before the async one. My guess is the former will be clearer rather than chaining pipes.

+1. @jyothishjose6190 since traceDetails itself is coming from an observable,htMemoize would be super useful here. It would only call the method if the parameter object changes. You will find few existing examples in the product.

Both this comment and the one about the link component came up in the PR to add this support on span attributes, so you could use that PR as a reference too (although there, the decision was made for styling reasons to keep the filter link and value rendered separately - not sure if the same applies here):

https://github.com/hypertrace/hypertrace-ui/pull/1079/files#diff-8912ac915c8ff140908e47b3997b34e34b6eb5a0defd418b07953b86980ba4c5R19-R24

anandtiwary · 2021-09-01T18:45:39Z

projects/observability/src/pages/trace-detail/trace-detail.page.component.ts

-            label="Trace ID"
-            [value]="traceDetails.id"
-          ></ht-summary-value>
+          <ng-container *ngIf="this.getExploreNavigationParams(traceDetails.id) | async as exploreNavigationParams">


Same changes here

jyothishjose6190 · 2021-09-02T10:19:28Z

@aaron-steinfeld @anandtiwary made changes. please review

aaron-steinfeld · 2021-09-02T13:00:26Z

projects/observability/src/pages/trace-detail/trace-detail.page.component.ts

@@ -82,4 +92,9 @@ export class TraceDetailPageComponent {
  public onClickBack(): void {
    this.navigationService.navigateBack();
  }
+
+  public getExplorerNavigationParams = (traceDetails: ApiTraceDetails | undefined): Observable<NavigationParams> =>


This is a full trace, but also doesn't appear like it can ever be called with undefined.

Suggested change

public getExplorerNavigationParams = (traceDetails: ApiTraceDetails | undefined): Observable<NavigationParams> =>

public getExplorerNavigationParams = (traceDetails: TraceDetails): Observable<NavigationParams> =>

aaron-steinfeld · 2021-09-02T13:00:47Z

projects/observability/src/pages/trace-detail/trace-detail.page.module.ts

    LabelModule,
    TracingDashboardModule,
    IconModule,
    SummaryValueModule,
+    LinkModule,


link no longer used

please remove it

aaron-steinfeld · 2021-09-02T13:01:10Z

projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts

@@ -86,4 +96,9 @@ export class ApiTraceDetailPageComponent {
  public navigateToFullTrace(traceId: string, startTime: string): void {
    this.navigationService.navigateWithinApp(['/trace', traceId, { startTime: startTime }]);
  }
+
+  public getExplorerNavigationParams = (traceDetails: ApiTraceDetails | undefined): Observable<NavigationParams> =>


Suggested change

public getExplorerNavigationParams = (traceDetails: ApiTraceDetails | undefined): Observable<NavigationParams> =>

public getExplorerNavigationParams = (traceDetails: ApiTraceDetails): Observable<NavigationParams> =>

aaron-steinfeld · 2021-09-02T13:01:55Z

.../observability/src/shared/dashboard/widgets/waterfall/waterfall-widget-renderer.component.ts

+          <ht-explore-filter-link
+            class="filter-link"
+            [paramsOrUrl]="getExploreNavigationParams | htMemoize: this.selectedData | async"
+            htTooltip="See traces in Explorer"


it looks like this is going to see spans, not traces

@jyothishjose6190 Please update this.

anandtiwary · 2021-09-07T15:00:46Z

...rvability/src/shared/dashboard/widgets/waterfall/waterfall-widget-renderer.component.test.ts

+      mockProvider(OverlayService),
+      mockProvider(ExplorerService)
+    ],
+    declarations: [MockComponent(WaterfallChartComponent), MockComponent(ExploreFilterLinkComponent)]


Can we add some tests for this? (and in the other components)

jyothishjose6190 · 2021-09-22T05:41:38Z

@anandtiwary @aaron-steinfeld please review this

itssharmasandeep

Instead of doing this

<ht-explore-filter-link
class="filter-link"
[paramsOrUrl]="getExplorerNavigationParams | htMemoize: traceDetails | async"
htTooltip="See traces in Explorer"
 >
  <ht-summary-value
    class="summary-value"
    icon="${IconType.TraceId}"
    label="Trace ID"
    [value]="traceDetails.traceId"
  ></ht-summary-value>
 </ht-explore-filter-link>

Please do this similar to this at all the places, with appropriate CSS

<div class="filterable-summary-value">
  <ht-summary-value
  class="summary-value"
  icon="${IconType.TraceId}"
  label="Trace ID"
  [value]="traceDetails.traceId"
  ></ht-summary-value>
  <ht-explore-filter-link
    class="filter-link"
    [paramsOrUrl]="getExplorerNavigationParams | htMemoize: traceDetails | async"
    htTooltip="See traces in Explorer"
  >
  </ht-explore-filter-link>
</div>

The first approach is creating some issues (like 2 tooltips for spans etc and it also doesn't look good). I think the behaviour should be similar like other place where we click on the filter button and then we filter things.

aaron-steinfeld · 2021-09-22T20:57:44Z

Instead of doing this

<ht-explore-filter-link
class="filter-link"
[paramsOrUrl]="getExplorerNavigationParams | htMemoize: traceDetails | async"
htTooltip="See traces in Explorer"
 >
  <ht-summary-value
    class="summary-value"
    icon="${IconType.TraceId}"
    label="Trace ID"
    [value]="traceDetails.traceId"
  ></ht-summary-value>
 </ht-explore-filter-link>

Please do this similar to this at all the places, with appropriate CSS

<div class="filterable-summary-value">
  <ht-summary-value
  class="summary-value"
  icon="${IconType.TraceId}"
  label="Trace ID"
  [value]="traceDetails.traceId"
  ></ht-summary-value>
  <ht-explore-filter-link
    class="filter-link"
    [paramsOrUrl]="getExplorerNavigationParams | htMemoize: traceDetails | async"
    htTooltip="See traces in Explorer"
  >
  </ht-explore-filter-link>
</div>

The first approach is creating some issues (like 2 tooltips for spans etc and it also doesn't look good). I think the behaviour should be similar like other place where we click on the filter button and then we filter things.

So conceptually, the first approach is better. The whole idea of a filter link component accepting content is exactly this. If it doesn't look good or has duplicate tooltips (is this because the component already has a tooltip and we're adding a second one here?), I'd say we should fix that.

itssharmasandeep · 2021-09-23T06:30:50Z

So conceptually, the first approach is better. The whole idea of a filter link component accepting content is exactly this. If it doesn't look good or has duplicate tooltips (is this because the component already has a tooltip and we're adding a second one here?), I'd say we should fix that.

But the problem I see with the current approach is, we have <ng-content> inside the anchor tag, which I think should not have. So problem it creating is we are able to click traceID/spanID as well with the filter icon (which I see as not a good UX), either we remove the filter icon if traceID is clickable or we make traceID non clickable.

IMO, The filter icon should only be responsible to click and filter, rather than whole component(content inside it)

jyothishjose6190 · 2021-09-23T07:03:31Z

Also on other screens, we have the same functionality. Clicking on the filter icon alone will apply the filter. I think the second approach suggested by @itssharmasandeep, matches the implementations in other screens

aaron-steinfeld · 2021-09-23T12:12:49Z

Also on other screens, we have the same functionality. Clicking on the filter icon alone will apply the filter. I think the second approach suggested by @itssharmasandeep, matches the implementations in other screens

So purely talking from a UX perspective, the behavior I'd expect for the code above (<filter-link><content/><filter-link>) is to render only the content, show up the filter icon next to it when the content is hovered, and only support clicking on the filter icon. Based on the screenshot, that sounds like what we're doing on that table.

If that's not how the filter link component behaves today, we should probably change that but it doesn't need to be part of this PR. The main goal is consistency in UX, consistency in code can follow after.

itssharmasandeep · 2021-09-23T12:44:54Z

So purely talking from a UX perspective, the behavior I'd expect for the code above () is to render only the content, show up the filter icon next to it when the content is hovered, and only support clicking on the filter icon. Based on the screenshot, that sounds like what we're doing on that table.
If that's not how the filter link component behaves today, we should probably change that but it doesn't need to be part of this PR. The main goal is consistency in UX, consistency in code can follow after.

I completely agree with you on this, when I was working on tag search then I implemented this hover effect (but not a part of filter link component) and it is not there inside it currently.
If we don't want to change filter link component here, then I think my approach would be better as it would give us consistency that we want right now, we can take filter link component change in a follow up PR.

aaron-steinfeld · 2021-09-23T13:23:43Z

If we don't want to change filter link component here, then I think my approach would be better as it would give us consistency that we want right now, we can take filter link component change in a follow up PR.

Yep, either way works for me. Doing it in this PR is probably quicker, but is a scope increase - but given its only used in one other spot that I see, not a significant one.

when I was working on tag search then I implemented this hover effect (but not a part of filter link component) and it is not there inside it currently.

I could not for the life of me figure out how that was working... turns out I haven't pulled in weeks and was comparing a UI with your changes and code without :(

jyothishjose6190 · 2021-09-24T06:48:38Z

@aaron-steinfeld @itssharmasandeep please review

itssharmasandeep

It looks good to me, works fine as well, as expected.
@jyothishjose6190 let @aaron-steinfeld also review then we can merge this

github-actions · 2021-09-27T07:00:11Z

Unit Test Results

    4 files ±0 269 suites +2 19m 13s ⏱️ + 2m 39s
966 tests +4 966 ✔️ +4 0 💤 ±0 0 ❌ ±0
973 runs +4 973 ✔️ +4 0 💤 ±0 0 ❌ ±0

Results for commit e680f9d. ± Comparison against base commit 594a8db.

jyothishjose6190 added 2 commits August 31, 2021 12:53

fix: trace drilldown to search

df3b19b

fix: lint errors

b9719c4

jyothishjose6190 requested a review from a team as a code owner August 31, 2021 09:56

aaron-steinfeld reviewed Aug 31, 2021

View reviewed changes

jyothishjose6190 added 2 commits August 31, 2021 17:57

fix: span drilldown

68a4ea6

fix: import issues fixed

660e6fc

anandtiwary reviewed Sep 1, 2021

View reviewed changes

jyothishjose6190 added 3 commits September 2, 2021 09:33

feat: search span id from waterfall chart

d00cf46

feat: search trace id

36d7814

fix: style clean up

51d9b3b

aaron-steinfeld reviewed Sep 2, 2021

View reviewed changes

anandtiwary reviewed Sep 7, 2021

View reviewed changes

jyothishjose6190 added 4 commits September 17, 2021 10:18

fix: trace, span search explorer - clean up

fa4ceb6

fix: added test cases for span and trace search

125cb26

fix: fixed lint errors

3030d2b

fix: import error

97af327

itssharmasandeep requested changes Sep 22, 2021

View reviewed changes

fix: trace, span search - ui clean up

58c5efd

itssharmasandeep self-requested a review September 24, 2021 11:51

itssharmasandeep approved these changes Sep 24, 2021

View reviewed changes

itssharmasandeep self-requested a review September 24, 2021 11:53

itssharmasandeep approved these changes Sep 24, 2021

View reviewed changes

aaron-steinfeld approved these changes Sep 24, 2021

View reviewed changes

Merge branch 'main' into trace_drilldown_search_1076

484cc8f

itssharmasandeep merged commit e680f9d into hypertrace:main Sep 27, 2021

	public getExplorerNavigationParams = (traceDetails: ApiTraceDetails \| undefined): Observable<NavigationParams> =>
	public getExplorerNavigationParams = (traceDetails: TraceDetails): Observable<NavigationParams> =>

Trace drilldown to explorer screen #1098

Trace drilldown to explorer screen #1098

Uh oh!

Conversation

jyothishjose6190 commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Checklist:

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

aaron-steinfeld 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!

jyothishjose6190 commented Sep 2, 2021

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!

jyothishjose6190 commented Sep 22, 2021

Uh oh!

itssharmasandeep left a comment

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld commented Sep 22, 2021

Uh oh!

itssharmasandeep commented Sep 23, 2021

Uh oh!

jyothishjose6190 commented Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaron-steinfeld commented Sep 23, 2021

Uh oh!

itssharmasandeep commented Sep 23, 2021

Uh oh!

aaron-steinfeld commented Sep 23, 2021

Uh oh!

jyothishjose6190 commented Sep 24, 2021

Uh oh!

itssharmasandeep left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 27, 2021

Unit Test Results

Uh oh!

Uh oh!

jyothishjose6190 commented Aug 31, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

jyothishjose6190 commented Sep 23, 2021 •

edited

Loading