Skip to content

feat: fetching log records and showing log icon #817

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 12 commits into from
May 6, 2021
Merged

feat: fetching log records and showing log icon #817

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
202b00c
feat: fetching log records and showing log icon
itssharmasandeep May 4, 2021
8f0b5c8
fix: adding trace start time to span data
itssharmasandeep May 4, 2021
533c904
Revert "fix: adding trace start time to span data"
itssharmasandeep May 4, 2021
a1d101d
fix: addressing review comments
itssharmasandeep May 4, 2021
8886ae2
fix: addressing review comments
itssharmasandeep May 6, 2021
11bee55
Revert "fix: addressing review comments"
itssharmasandeep May 6, 2021
77d168d
fix: addressing review comments
itssharmasandeep May 6, 2021
129b6a4
fix: test cases
itssharmasandeep May 6, 2021
929f90b
Merge branch 'main' into log-icon
itssharmasandeep May 6, 2021
b45756d
fix: addressing review comments
itssharmasandeep May 6, 2021
01b9bbc
fix: lint errors
itssharmasandeep May 6, 2021
e25a66c
Merge remote-tracking branch 'origin/log-icon' into log-icon
itssharmasandeep May 6, 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 projects/distributed-tracing/src/public-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ export { WaterfallData } from './shared/dashboard/widgets/waterfall/waterfall/wa
export { TraceWaterfallDataSourceModel } from './shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model';
export { traceDetailDashboard } from './pages/trace-detail/trace-detail.dashboard';
export { TraceDetailPageComponent } from './pages/trace-detail/trace-detail.page.component';
export { LogEvent } from './shared/dashboard/widgets/waterfall/waterfall/waterfall-chart';

// Datasources
export * from './shared/dashboard/widgets/trace-detail/data/trace-detail-data-source.model';
Expand Down
3 changes: 3 additions & 0 deletions projects/distributed-tracing/src/shared/components/span-detail/span-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Dictionary } from '@hypertrace/common';
import { LogEvent } from '../../dashboard/widgets/waterfall/waterfall/waterfall-chart';

export interface SpanData {
id: string;
Expand All @@ -12,4 +13,6 @@ export interface SpanData {
tags: Dictionary<unknown>;
requestUrl: string;
exitCallsBreakup?: Dictionary<string>;
startTime?: number;
Copy link
Contributor

@jake-bassett jake-bassett May 5, 2021

Choose a reason for hiding this comment

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

Surprised startTime isn't required. Is there a reason some of these properties optional?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep May 6, 2021

Choose a reason for hiding this comment

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

For startTime I've fixed. logEvents can be undefined. Apart from these I do not have much idea. may be same reason for others as well.

logEvents?: LogEvent[];
}
34 changes: 34 additions & 0 deletions ...ing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,23 @@ describe('Trace Waterfall data source model', () => {
expect.objectContaining({
name: 'errorCount'
})
],
logEventProperties: [
expect.objectContaining({
name: 'traceId'
}),
expect.objectContaining({
name: 'attributes'
}),
expect.objectContaining({
name: 'timestamp'
}),
expect.objectContaining({
name: 'spanId'
}),
expect.objectContaining({
name: 'summary'
})
]
}
});
Expand Down Expand Up @@ -156,6 +173,23 @@ describe('Trace Waterfall data source model', () => {
expect.objectContaining({
name: 'errorCount'
})
],
logEventProperties: [
expect.objectContaining({
name: 'traceId'
}),
expect.objectContaining({
name: 'attributes'
}),
expect.objectContaining({
name: 'timestamp'
}),
expect.objectContaining({
name: 'spanId'
}),
expect.objectContaining({
name: 'summary'
})
]
}
});
Expand Down
16 changes: 13 additions & 3 deletions ...-tracing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
TRACE_GQL_REQUEST
} from '../../../../graphql/request/handlers/traces/trace-graphql-query-handler.service';
import { MetadataService } from '../../../../services/metadata/metadata.service';
import { WaterfallData } from '../../../widgets/waterfall/waterfall/waterfall-chart';
import { LogEvent, WaterfallData } from '../../../widgets/waterfall/waterfall/waterfall-chart';
import { GraphQlDataSourceModel } from '../graphql-data-source.model';

@Model({
Expand Down Expand Up @@ -60,6 +60,14 @@ export class TraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Waterf
this.specificationBuilder.attributeSpecificationForKey('errorCount')
];

protected readonly logEventSpecifications: Specification[] = [
this.specificationBuilder.attributeSpecificationForKey('traceId'),
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 6, 2021

Choose a reason for hiding this comment

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

nit: do we actually need the spanid and trace id in these? isn't it already available in the span parent?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep May 6, 2021

Choose a reason for hiding this comment

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

yeah, we don't need that actually.
thank you for pointing that out.

this.specificationBuilder.attributeSpecificationForKey('attributes'),
this.specificationBuilder.attributeSpecificationForKey('timestamp'),
this.specificationBuilder.attributeSpecificationForKey('spanId'),
this.specificationBuilder.attributeSpecificationForKey('summary')
];

public getData(): Observable<WaterfallData[]> {
return combineLatest([this.getTraceData(), this.getDurationAttribute()]).pipe(
map(combinedData => this.mapResponseObject(combinedData[0], combinedData[1]))
Expand All @@ -73,7 +81,8 @@ export class TraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Waterf
spanLimit: 1000,
timestamp: this.dateCoercer.coerce(this.startTime),
traceProperties: [],
spanProperties: this.spanSpecifications
spanProperties: this.spanSpecifications,
logEventProperties: this.logEventSpecifications
});
}

Expand Down Expand Up @@ -102,7 +111,8 @@ export class TraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Waterf
apiName: span.displaySpanName as string,
spanType: span.type as SpanType,
tags: span.spanTags as Dictionary<unknown>,
errorCount: span.errorCount as number
errorCount: span.errorCount as number,
logEvents: ((span.logEvents as Dictionary<LogEvent[]>) ?? {}).results
}));
}
}
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 @@ -4,4 +4,5 @@ export interface SpanNameCellData {
apiName?: string;
color?: string;
hasError?: boolean;
hasLogs?: boolean;
}
6 changes: 5 additions & 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 min-content auto;
grid-template-columns: 3px min-content min-content min-content min-content auto;
grid-template-rows: 1fr;
column-gap: 4px;

Expand Down Expand Up @@ -41,6 +41,10 @@
.text {
@include ellipsis-overflow();
}

.log-icon {
margin: auto 0;
}
}

.clickable {
Expand Down
11 changes: 11 additions & 0 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 @@ -61,4 +61,15 @@ describe('Span name table cell renderer component', () => {
expect(spectator.query('.color-bar')).toExist();
expect(spectator.query('.error-icon')).toExist();
});

test('should render log icon ', () => {
const spanNameDataWithColor = {
...spanNameData,
hasLogs: true
};
const spectator = buildComponent({
providers: [tableCellDataProvider(spanNameDataWithColor)]
});
expect(spectator.query('.log-icon')).toExist();
});
});
7 changes: 7 additions & 0 deletions ...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
Expand Up @@ -28,6 +28,13 @@ import { WaterfallTableCellType } from './span-name-cell-type';
size="${IconSize.Medium}"
color="${Color.Red5}"
></ht-icon>
<ht-icon
*ngIf="this.value.hasLogs"
class="log-icon"
icon="${IconType.Note}"
size="${IconSize.Small}"
color="${Color.Gray4}"
></ht-icon>
</div>
`
})
Expand Down
3 changes: 2 additions & 1 deletion ...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 @@ -42,7 +42,8 @@ export class WaterfallChartService {
apiName: datum.apiName,
serviceName: datum.serviceName,
protocolName: datum.protocolName,
hasError: datum.errorCount > 0
hasError: datum.errorCount > 0,
hasLogs: datum.logEvents && datum.logEvents.length > 0
},
$$iconType: this.iconLookupService.forSpanType(datum.spanType)!,
getChildren: () => of([]),
Expand Down
10 changes: 10 additions & 0 deletions ...s/distributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export interface WaterfallData {
responseBody?: string;
tags: Dictionary<unknown>;
errorCount: number;
logEvents?: LogEvent[];
Copy link
Contributor

@jake-bassett jake-bassett May 5, 2021

Choose a reason for hiding this comment

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

Sort of the same question. Should this be optional? I guess in this case maybe we could expect logEvents to be undefined if none are associated with the span?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep May 6, 2021

Choose a reason for hiding this comment

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

Yeah, this can be the case.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 6, 2021

Choose a reason for hiding this comment

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

logEvents will never return null, always an empty array - unless we're conditionally requesting it (but I'd argue in this context, we should ensure it's there for simplicity downstream).

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep May 6, 2021

Choose a reason for hiding this comment

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

Added extra condition to handle that.
So it is not optional anymore.

}

export interface WaterfallDataNode extends WaterfallData, Omit<StatefulPrefetchedTreeTableRow, '$$state'> {
Expand All @@ -37,3 +38,12 @@ export interface WaterfallChartState {
children: WaterfallDataNode[];
expanded: boolean;
}

export interface LogEvent {
[key: string]: unknown;
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld May 6, 2021

Choose a reason for hiding this comment

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

what unknown fields does this have? would anything break if you removed this?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep May 6, 2021

Choose a reason for hiding this comment

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

Actually I added this as I was using this as a table row, but lately we did not use that so now removed it.
Fixed!!

traceId: string;
spanId: string;
attributes: Dictionary<unknown>;
timestamp: string;
summary: string;
}
22 changes: 20 additions & 2 deletions ...tracing/src/shared/graphql/request/handlers/traces/trace-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,23 @@ export class TraceGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
children: [
{
path: 'results',
children: [{ path: 'id' }, ...this.selectionBuilder.fromSpecifications(request.spanProperties!)]
children: [
{ path: 'id' },
...(!isEmpty(request.logEventProperties)
? [
{
path: 'logEvents',
children: [
{
path: 'results',
children: this.selectionBuilder.fromSpecifications(request.logEventProperties!)
}
]
}
]
: []),
...this.selectionBuilder.fromSpecifications(request.spanProperties!)
]
}
]
};
Expand Down Expand Up @@ -121,7 +137,8 @@ export class TraceGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap

rawResult.results.forEach(spanRawResult => {
const span: Span = {
[spanIdKey]: spanRawResult.id as string
[spanIdKey]: spanRawResult.id as string,
...(!isEmpty(spanRawResult.logEvents) && { logEvents: spanRawResult.logEvents })
};

(request.spanProperties || []).forEach(property => {
Expand Down Expand Up @@ -175,6 +192,7 @@ export interface GraphQlTraceRequest {
spanLimit: number;
spanId?: string;
spanProperties?: Specification[];
logEventProperties?: Specification[];
}

interface TraceServerResponse {
Expand Down
34 changes: 34 additions & 0 deletions ...src/shared/dashboard/data/graphql/waterfall/api-trace-waterfall-data-source.model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ describe('Api Trace Waterfall data source model', () => {
expect.objectContaining({
name: 'errorCount'
})
],
logEventProperties: [
expect.objectContaining({
name: 'traceId'
}),
expect.objectContaining({
name: 'attributes'
}),
expect.objectContaining({
name: 'timestamp'
}),
expect.objectContaining({
name: 'spanId'
}),
expect.objectContaining({
name: 'summary'
})
]
}
});
Expand Down Expand Up @@ -163,6 +180,23 @@ describe('Api Trace Waterfall data source model', () => {
expect.objectContaining({
name: 'errorCount'
})
],
logEventProperties: [
expect.objectContaining({
name: 'traceId'
}),
expect.objectContaining({
name: 'attributes'
}),
expect.objectContaining({
name: 'timestamp'
}),
expect.objectContaining({
name: 'spanId'
}),
expect.objectContaining({
name: 'summary'
})
]
}
});
Expand Down
11 changes: 10 additions & 1 deletion ...lity/src/shared/dashboard/data/graphql/waterfall/api-trace-waterfall-data-source.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { DateCoercer, Dictionary } from '@hypertrace/common';
import {
AttributeMetadata,
GraphQlDataSourceModel,
LogEvent,
MetadataService,
Span,
spanIdKey,
Expand Down Expand Up @@ -65,6 +66,10 @@ export class ApiTraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Wat
];
}

private getLogEventAttributes(): string[] {
return ['traceId', 'attributes', 'timestamp', 'spanId', 'summary'];
}

private getTraceData(): Observable<Trace | undefined> {
return this.query<TraceGraphQlQueryHandlerService>({
requestType: TRACE_GQL_REQUEST,
Expand All @@ -75,6 +80,9 @@ export class ApiTraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Wat
traceProperties: [],
spanProperties: this.getSpanAttributes().map(attribute =>
this.specificationBuilder.attributeSpecificationForKey(attribute)
),
logEventProperties: this.getLogEventAttributes().map(attribute =>
this.specificationBuilder.attributeSpecificationForKey(attribute)
)
});
}
Expand Down Expand Up @@ -108,7 +116,8 @@ export class ApiTraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Wat
apiName: span.displaySpanName as string,
spanType: span.type as SpanType,
tags: span.spanTags as Dictionary<unknown>,
errorCount: span.errorCount as number
errorCount: span.errorCount as number,
logEvents: ((span.logEvents as Dictionary<LogEvent[]>) ?? {}).results
};
}
}