Skip to content

feat: highlight error span on partial or full error #773

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
merged 14 commits into from
Apr 22, 2021
Merged

feat: highlight error span on partial or full error #773

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b1ce950
feat: highlight error span or traces on partial or full error
itssharmasandeep Apr 15, 2021
a407cf3
Merge remote-tracking branch 'origin/main' into span-error
itssharmasandeep Apr 15, 2021
254a519
Merge remote-tracking branch 'origin/main' into span-error
itssharmasandeep Apr 16, 2021
0593e3f
feat: adding errors column to trace lists
itssharmasandeep Apr 19, 2021
4f83aaa
refactor: removing unwanted code
itssharmasandeep Apr 19, 2021
bc46627
Merge branch 'main' into span-error
itssharmasandeep Apr 19, 2021
db30235
Merge branch 'main' into span-error
itssharmasandeep Apr 20, 2021
9ba8708
fix: addressing review comments
itssharmasandeep Apr 20, 2021
fd9df26
Merge branch 'main' into span-error
itssharmasandeep Apr 21, 2021
150943f
fix: using errorcount attribute
itssharmasandeep Apr 21, 2021
6b75bdb
Merge branch 'span-error' of github.com:hypertrace/hypertrace-ui into…
itssharmasandeep Apr 21, 2021
c3f9b0a
Merge branch 'main' into span-error
itssharmasandeep Apr 22, 2021
093f57f
fix: test cases
itssharmasandeep Apr 22, 2021
c6898e5
refactor: addressing review comments
itssharmasandeep Apr 22, 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
1 change: 1 addition & 0 deletions ...tracing/src/shared/dashboard/widgets/waterfall/waterfall/span-name/span-name-cell-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ export interface SpanNameCellData {
protocolName: string;
name: string;
color?: string;
error?: boolean;
}
2 changes: 1 addition & 1 deletion ...hboard/widgets/waterfall/waterfall/span-name/span-name-table-cell-renderer.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

.span-title {
display: grid;
grid-template-columns: 3px min-content min-content auto;
grid-template-columns: 3px min-content min-content min-content auto;
grid-template-rows: 1fr;
column-gap: 4px;

Expand Down
9 changes: 6 additions & 3 deletions ...ard/widgets/waterfall/waterfall/span-name/span-name-table-cell-renderer.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('Span name table cell renderer component', () => {
shallow: true
});

test('should render span name without color and build tooltip ', () => {
test('should render span name without color and error icon and build tooltip ', () => {
const spectator = buildComponent();

const tooltip = `${spanNameData.serviceName} ${spanNameData.protocolName} ${spanNameData.name}`;
Expand All @@ -38,12 +38,14 @@ describe('Span name table cell renderer component', () => {
expect(spectator.query('.protocol-name')).toHaveText('test-protocol');
expect(spectator.query('.span-name')).toHaveText('test-span-name');
expect(spectator.query('.color-bar')).not.toExist();
expect(spectator.query('.error-icon')).not.toExist();
});

test('should render span name with color and build tooltip ', () => {
test('should render span name with color and error icon and build tooltip ', () => {
const spanNameDataWithColor = {
...spanNameData,
color: 'blue'
color: 'blue',
error: true
};
const spectator = buildComponent({
providers: [tableCellDataProvider(spanNameDataWithColor)]
Expand All @@ -57,5 +59,6 @@ describe('Span name table cell renderer component', () => {
expect(spectator.query('.protocol-name')).toHaveText('test-protocol');
expect(spectator.query('.span-name')).toHaveText('test-span-name');
expect(spectator.query('.color-bar')).toExist();
expect(spectator.query('.error-icon')).toExist();
});
});
11 changes: 10 additions & 1 deletion ...ashboard/widgets/waterfall/waterfall/span-name/span-name-table-cell-renderer.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { ChangeDetectionStrategy, Component } from '@angular/core';
import { TableCellAlignmentType, TableCellRenderer, TableCellRendererBase } from '@hypertrace/components';
import { IconType } from '@hypertrace/assets-library';
import { Color } from '@hypertrace/common';
import { IconSize, TableCellAlignmentType, TableCellRenderer, TableCellRendererBase } from '@hypertrace/components';
import { SpanNameCellData } from './span-name-cell-data';
import { WaterfallTableCellType } from './span-name-cell-type';

Expand All @@ -19,6 +21,13 @@ import { WaterfallTableCellType } from './span-name-cell-type';
<div class="span-name">
<span class="text" data-sensitive-pii>{{ this.value.name }}</span>
</div>
<ht-icon
*ngIf="this.value.error"
class="error-icon"
icon="${IconType.Error}"
size="${IconSize.Medium}"
color="${Color.Red5}"
></ht-icon>
</div>
`
})
Expand Down
7 changes: 4 additions & 3 deletions ...ibuted-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { SequenceChartModule, TableModule, TooltipModule } from '@hypertrace/components';
import { IconModule, SequenceChartModule, TableModule, TooltipModule } from '@hypertrace/components';
import { SpanNameTableCellParser } from './span-name/span-name-table-cell-parser';
import { SpanNameTableCellRendererComponent } from './span-name/span-name-table-cell-renderer.component';
import { WaterfallChartComponent } from './waterfall-chart.component';
Expand All @@ -12,8 +12,9 @@ import { WaterfallChartComponent } from './waterfall-chart.component';
CommonModule,
TableModule.withCellParsers([SpanNameTableCellParser]),
TableModule.withCellRenderers([SpanNameTableCellRendererComponent]),
TooltipModule
TooltipModule,
IconModule
],
exports: [WaterfallChartComponent]
exports: [WaterfallChartComponent, SpanNameTableCellRendererComponent]
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 19, 2021

Choose a reason for hiding this comment

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

This shouldn't need to be exported since we don't directly reference renderers - did you run into an issue?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

I guess, It was a mistake.
Fixed

})
export class WaterfallChartModule {}
22 changes: 13 additions & 9 deletions ...buted-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ export class WaterfallChartService {
$$spanName: {
name: datum.name,
serviceName: datum.serviceName,
protocolName: datum.protocolName
protocolName: datum.protocolName,
error: datum.tags?.error as boolean
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 19, 2021

Choose a reason for hiding this comment

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

isn't the error state a top level attribute? should we be fetching it directly rather than through the tag map (which means that it would be populated potentially via enrichment, since there's more than one way to mark an error).

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

I don't think it is a top level attribute. Whenever I console log the data, I did not see any error attribute but there is one in the tags.
May be I am missing something.
cc: @kotharironak

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 20, 2021

Choose a reason for hiding this comment

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

The top level attributes need to be added to the request, too. See the various waterfall data source models (for example from trace-waterfall-data-source.model.ts:

  protected readonly spanSpecifications: Specification[] = [
    this.specificationBuilder.attributeSpecificationForKey('displaySpanName'),
    this.specificationBuilder.attributeSpecificationForKey('duration'),
    this.specificationBuilder.attributeSpecificationForKey('endTime'),
    this.specificationBuilder.attributeSpecificationForKey('parentSpanId'),
    this.specificationBuilder.attributeSpecificationForKey('serviceName'),
    this.specificationBuilder.attributeSpecificationForKey('spanTags'),
    this.specificationBuilder.attributeSpecificationForKey('startTime'),
    this.specificationBuilder.attributeSpecificationForKey('type'),
    this.specificationBuilder.attributeSpecificationForKey('traceId')
  ];

Copy link

@kotharironak kotharironak Apr 21, 2021

Choose a reason for hiding this comment

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

@itssharmasandeep we should be using EVENT. errorCount > 0 to indicate error. This value of this attribute is based on tags that indicate error at span level.
https://github.com/hypertrace/config-bootstrapper/blob/main/config-bootstrapper/src/main/resources/configs/config-bootstrapper/attribute-service/hypertrace/hypertrace_span_attributes.conf#L62

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 21, 2021

Choose a reason for hiding this comment

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

Yeah, I am using the same now.
updated the PR.

},
$$iconType: this.iconLookupService.forSpanType(datum.spanType)!,
getChildren: () => of([]),
Expand Down Expand Up @@ -82,7 +83,6 @@ export class WaterfallChartService {
// Do DFS
while (sequenceNodes.length !== 0) {
const node = sequenceNodes.shift()!;

if (node.$$state.expanded) {
segments.push({
id: node.id,
Expand Down Expand Up @@ -123,14 +123,18 @@ export class WaterfallChartService {
const node = nodes.shift()!;
let color;

if (colorMap.has(node.serviceName)) {
// ServiceName seen before. Use existing service color
color = colorMap.get(node.serviceName)!;
if (node.tags?.error as boolean) {
color = Color.Red5; // If span conatins an error
} else {
// New serviceName. Assign a new color
color = originalColors[uniqueServiceIndex % originalColors.length];
colorMap.set(node.serviceName, color);
uniqueServiceIndex++;
if (colorMap.has(node.serviceName)) {
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 19, 2021

Choose a reason for hiding this comment

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

extra nest (use else if)

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

Fixed

// ServiceName seen before. Use existing service color
color = colorMap.get(node.serviceName)!;
} else {
// New serviceName. Assign a new color
color = originalColors[uniqueServiceIndex % originalColors.length];
colorMap.set(node.serviceName, color);
uniqueServiceIndex++;
}
}

node.color = color;
Expand Down
13 changes: 13 additions & 0 deletions projects/observability/src/pages/apis/api-detail/traces/api-trace-list.dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,19 @@ export const apiTraceListDashboard = {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Errors',
width: '180px',
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 19, 2021

Choose a reason for hiding this comment

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

This is just a number right? 180px feels a bit excessive

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

Please refer to: #786

filterable: true,
value: {
type: 'attribute-specification',
attribute: 'errorCount'
},
'click-handler': {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Duration',
Expand Down
13 changes: 13 additions & 0 deletions projects/observability/src/pages/apis/service-detail/traces/service-trace-list.dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,19 @@ export const serviceTraceListDashboard = {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Errors',
width: '180px',
filterable: true,
value: {
type: 'attribute-specification',
attribute: 'errorCount'
},
'click-handler': {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Duration',
Expand Down
13 changes: 13 additions & 0 deletions projects/observability/src/pages/explorer/explorer-dashboard-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,19 @@ export class ExplorerDashboardBuilder {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Errors',
width: '180px',
filterable: true,
value: {
type: 'attribute-specification',
attribute: 'errorCount'
Copy link

@kotharironak kotharironak Apr 19, 2021

Choose a reason for hiding this comment

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

I think the name of an attribute is apiTraceErrorSpanCount in API_TRACE scop
cc: @JBAhire

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 19, 2021

Choose a reason for hiding this comment

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

While using the attribute named errorCount, I am getting the data but not getting any data while using apiTraceErrorSpanCount

Copy link
Member

@JBAhire JBAhire Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's weird because I can see data in Pinot as well as filters on explorer and service screen. Maybe the data you are trying with doesn't have any error spans in API trace?

Screenshot 2021-04-19 at 6 29 43 PM

Screenshot 2021-04-19 at 6 29 28 PM

Screenshot 2021-04-19 at 6 28 57 PM

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Apr 19, 2021

Choose a reason for hiding this comment

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

errorCount is a different thing - it will be 1 if the api trace is overall in error, 0 otherwise (used for calculating error metrics)

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

It does work with apiTraceErrorSpanCount now.

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 20, 2021

Choose a reason for hiding this comment

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

referennce: #786

},
'click-handler': {
type: 'api-trace-navigation-handler'
}
},
{
type: 'table-widget-column',
title: 'Duration',
Expand Down