Skip to content

Trace drilldown to explorer screen #1098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
itssharmasandeep merged 13 commits into from
Sep 27, 2021
Merged

Trace drilldown to explorer screen #1098

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
df3b19b
fix: trace drilldown to search
jyothishjose6190 Aug 31, 2021
b9719c4
fix: lint errors
jyothishjose6190 Aug 31, 2021
68a4ea6
fix: span drilldown
jyothishjose6190 Aug 31, 2021
660e6fc
fix: import issues fixed
jyothishjose6190 Sep 1, 2021
d00cf46
feat: search span id from waterfall chart
jyothishjose6190 Sep 2, 2021
36d7814
feat: search trace id
jyothishjose6190 Sep 2, 2021
51d9b3b
fix: style clean up
jyothishjose6190 Sep 2, 2021
fa4ceb6
fix: trace, span search explorer - clean up
jyothishjose6190 Sep 17, 2021
125cb26
fix: added test cases for span and trace search
jyothishjose6190 Sep 21, 2021
3030d2b
fix: fixed lint errors
jyothishjose6190 Sep 21, 2021
97af327
fix: import error
jyothishjose6190 Sep 21, 2021
58c5efd
fix: trace, span search - ui clean up
jyothishjose6190 Sep 24, 2021
484cc8f
Merge branch 'main' into trace_drilldown_search_1076
itssharmasandeep Sep 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { ChangeDetectionStrategy, Component } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { NavigationService, SubscriptionLifecycle } from '@hypertrace/common';
import { ButtonRole, ButtonStyle, IconSize } from '@hypertrace/components';
import { NavigationParams, NavigationService, SubscriptionLifecycle } from '@hypertrace/common';
import { ButtonRole, ButtonStyle, FilterOperator, IconSize } from '@hypertrace/components';
import { Observable } from 'rxjs';
import { LogEvent } from '../../shared/dashboard/widgets/waterfall/waterfall/waterfall-chart';
import { ExplorerService } from '../explorer/explorer-service';
import { ScopeQueryParam } from '../explorer/explorer.component';
import { ApiTraceDetails, ApiTraceDetailService } from './api-trace-detail.service';

@Component({
Expand Down Expand Up @@ -31,12 +33,19 @@ import { ApiTraceDetails, ApiTraceDetailService } from './api-trace-detail.servi
icon="${IconType.Time}"
[value]="traceDetails.timeString"
></ht-summary-value>
<ht-summary-value
class="summary-value"
icon="${IconType.TraceId}"
label="Trace ID"
[value]="traceDetails.traceId"
></ht-summary-value>

<ng-container
*ngIf="this.getExploreNavigationParams(traceDetails.traceId) | async as exploreNavigationParams"
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Aug 31, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@anandtiwary anandtiwary Sep 1, 2021

Choose a reason for hiding this comment

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

+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.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Sep 1, 2021

Choose a reason for hiding this comment

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

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

>
<ht-link class="link" [paramsOrUrl]="exploreNavigationParams">
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Aug 31, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Sep 1, 2021

Choose a reason for hiding this comment

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

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>

Copy link
Contributor

@anandtiwary anandtiwary Sep 1, 2021

Choose a reason for hiding this comment

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

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.

<ht-summary-value
class="summary-value"
icon="${IconType.TraceId}"
label="Trace ID"
[value]="traceDetails.traceId"
></ht-summary-value>
</ht-link>
</ng-container>

<div class="separation"></div>

Expand Down Expand Up @@ -73,7 +82,8 @@ export class ApiTraceDetailPageComponent {

public constructor(
protected readonly navigationService: NavigationService,
private readonly apiTraceDetailService: ApiTraceDetailService
private readonly apiTraceDetailService: ApiTraceDetailService,
private readonly explorerService: ExplorerService
) {
this.traceDetails$ = this.apiTraceDetailService.fetchTraceDetails();
this.logEvents$ = this.apiTraceDetailService.fetchLogEvents();
Expand All @@ -86,4 +96,10 @@ export class ApiTraceDetailPageComponent {
public navigateToFullTrace(traceId: string, startTime: string): void {
this.navigationService.navigateWithinApp(['/trace', traceId, { startTime: startTime }]);
}

public getExploreNavigationParams(traceId: string): Observable<NavigationParams> {
return this.explorerService.buildNavParamsWithFilters(ScopeQueryParam.EndpointTraces, [
{ field: 'traceId', operator: FilterOperator.Equals, value: traceId }
]);
}
}
2 changes: 2 additions & 0 deletions projects/observability/src/pages/api-trace-detail/api-trace-detail.page.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
CopyShareableLinkToClipboardModule,
IconModule,
LabelModule,
LinkModule,
LoadAsyncModule,
NavigableTabModule,
SummaryValueModule
Expand Down Expand Up @@ -53,6 +54,7 @@ const ROUTE_CONFIG: HtRoute[] = [
LoadAsyncModule,
FormattingModule,
ButtonModule,
LinkModule,
CopyShareableLinkToClipboardModule,
NavigableTabModule,
LogEventsTableModule,
Expand Down
31 changes: 22 additions & 9 deletions projects/observability/src/pages/trace-detail/trace-detail.page.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { ChangeDetectionStrategy, Component } from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { NavigationService, SubscriptionLifecycle } from '@hypertrace/common';
import { IconSize } from '@hypertrace/components';
import { NavigationParams, NavigationService, SubscriptionLifecycle } from '@hypertrace/common';
import { FilterOperator, IconSize } from '@hypertrace/components';
import { Observable } from 'rxjs';
import { LogEvent } from '../../shared/dashboard/widgets/waterfall/waterfall/waterfall-chart';
import { ExplorerService } from '../explorer/explorer-service';
import { ScopeQueryParam } from '../explorer/explorer.component';
import { TraceDetails, TraceDetailService } from './trace-detail.service';
@Component({
styleUrls: ['./trace-detail.page.component.scss'],
Expand All @@ -30,12 +32,16 @@ import { TraceDetails, TraceDetailService } from './trace-detail.service';
icon="${IconType.Time}"
[value]="traceDetails.timeString"
></ht-summary-value>
<ht-summary-value
class="summary-value"
icon="${IconType.TraceId}"
label="Trace ID"
[value]="traceDetails.id"
></ht-summary-value>
<ng-container *ngIf="this.getExploreNavigationParams(traceDetails.id) | async as exploreNavigationParams">
Copy link
Contributor

@anandtiwary anandtiwary Sep 1, 2021

Choose a reason for hiding this comment

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

Same changes here

<ht-link class="link" [paramsOrUrl]="exploreNavigationParams">
<ht-summary-value
class="summary-value"
icon="${IconType.TraceId}"
label="Trace ID"
[value]="traceDetails.id"
></ht-summary-value>
</ht-link>
</ng-container>

<div class="separation"></div>

Expand Down Expand Up @@ -72,7 +78,8 @@ export class TraceDetailPageComponent {

public constructor(
private readonly navigationService: NavigationService,
private readonly traceDetailService: TraceDetailService
private readonly traceDetailService: TraceDetailService,
private readonly explorerService: ExplorerService
) {
this.traceDetails$ = this.traceDetailService.fetchTraceDetails();
this.exportSpans$ = this.traceDetailService.fetchExportSpans();
Expand All @@ -82,4 +89,10 @@ export class TraceDetailPageComponent {
public onClickBack(): void {
this.navigationService.navigateBack();
}

public getExploreNavigationParams(traceId: string): Observable<NavigationParams> {
return this.explorerService.buildNavParamsWithFilters(ScopeQueryParam.EndpointTraces, [
{ field: 'traceId', operator: FilterOperator.Equals, value: traceId }
]);
}
}
2 changes: 2 additions & 0 deletions projects/observability/src/pages/trace-detail/trace-detail.page.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
DownloadJsonModule,
IconModule,
LabelModule,
LinkModule,
LoadAsyncModule,
NavigableTabModule,
SummaryValueModule,
Expand Down Expand Up @@ -51,6 +52,7 @@ const ROUTE_CONFIG: HtRoute[] = [
TracingDashboardModule,
IconModule,
SummaryValueModule,
LinkModule,
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Sep 2, 2021

Choose a reason for hiding this comment

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

link no longer used

Copy link
Contributor

@anandtiwary anandtiwary Sep 7, 2021

Choose a reason for hiding this comment

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

please remove it

TooltipModule,
LoadAsyncModule,
FormattingModule,
Expand Down