Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
202b00c
feat: fetching log records and showing log icon
itssharmasandeep May 4, 2021
2e7f58b
feat: log detail widget and log timestamp table cell renderer components
itssharmasandeep May 4, 2021
3cb722e
fix: adding trace start time
itssharmasandeep May 4, 2021
8f0b5c8
fix: adding trace start time to span data
itssharmasandeep May 4, 2021
b03c83d
Merge branch 'log-icon' into log-components
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
8033540
Revert "fix: adding trace start time"
itssharmasandeep May 4, 2021
cd1214b
Revert "fix: adding trace start time to span data"
itssharmasandeep May 4, 2021
e51b716
Merge branch 'log-icon' into log-components
itssharmasandeep May 4, 2021
e2aef98
fix: changing log-timestamp to relative timestamp
itssharmasandeep May 5, 2021
552f7bc
fix: addressing review comments
itssharmasandeep May 5, 2021
2e642b8
refactor: removing unwanted changes
itssharmasandeep May 5, 2021
2e90a3a
fix: lint errors
itssharmasandeep May 5, 2021
171dc8a
fix: adding and fixing tests
itssharmasandeep May 5, 2021
36e8d44
fix: address review comment and test fix
itssharmasandeep May 5, 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
fe85d1c
Merge branch 'log-icon' into log-components
itssharmasandeep May 6, 2021
129b6a4
fix: test cases
itssharmasandeep May 6, 2021
8466606
Merge branch 'log-icon' into log-components
itssharmasandeep May 6, 2021
10f3d37
fix: test case
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
2874cca
Merge branch 'log-icon' into log-components
itssharmasandeep May 6, 2021
88f233a
fix: test cases
itssharmasandeep May 6, 2021
411ab62
Merge branch 'main' into log-components
itssharmasandeep May 6, 2021
5a4126a
Merge remote-tracking branch origin/main into log-components
itssharmasandeep May 7, 2021
08ba01b
Merge branch 'log-components' of github.com:hypertrace/hypertrace-ui …
itssharmasandeep May 7, 2021
c5b8a42
fix: creating table instead of widget renderer
itssharmasandeep May 7, 2021
29f2bdd
feat: log records in the span sheet (#819)
itssharmasandeep May 7, 2021
bf4b7ae
Merge branch 'main' into log-components
itssharmasandeep May 7, 2021
dd1b677
Merge remote-tracking branch 'origin/main' into log-components
itssharmasandeep May 11, 2021
e0b2ef4
fix: addressing review comments
itssharmasandeep May 11, 2021
2b87274
Merge branch 'main' into log-components
itssharmasandeep May 12, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@import 'font';

.relative-timestamp {
@include ellipsis-overflow();

&.first-column {
@include body-1-medium($gray-9);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {
TableCellNoOpParser,
tableCellProviders,
tableCellRowDataProvider,
TooltipDirective
} from '@hypertrace/components';
import { createComponentFactory } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { tableCellDataProvider } from '../../test/cell-providers';
import {
RelativeTimestampTableCellRendererComponent,
RowData
} from './relative-timestamp-table-cell-renderer.component';

describe('log timestamp table cell renderer component', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: relative timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!!

const buildComponent = createComponentFactory({
component: RelativeTimestampTableCellRendererComponent,
providers: [
tableCellProviders(
{
id: 'test'
},
new TableCellNoOpParser(undefined!)
)
],
declarations: [MockComponent(TooltipDirective)],
shallow: true
});

test('testing component properties', () => {
const logEvent: RowData = {
baseTimestamp: 1619785437887
};
const spectator = buildComponent({
providers: [tableCellRowDataProvider(logEvent), tableCellDataProvider('2021-04-30T12:23:57.889149Z')]
});

expect(spectator.queryAll('.relative-timestamp')[0]).toContainText('2 ms');
expect(spectator.component.duration).toBe(2);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { ChangeDetectionStrategy, Component, Inject, OnInit } from '@angular/core';
import { DateCoercer, DateFormatMode, DateFormatOptions } from '@hypertrace/common';
import { TableColumnConfig } from '../../../table-api';
import {
TABLE_CELL_DATA,
TABLE_COLUMN_CONFIG,
TABLE_COLUMN_INDEX,
TABLE_DATA_PARSER,
TABLE_ROW_DATA
} from '../../table-cell-injection';
import { TableCellParserBase } from '../../table-cell-parser-base';
import { TableCellRenderer } from '../../table-cell-renderer';
import { TableCellRendererBase } from '../../table-cell-renderer-base';
import { CoreTableCellParserType } from '../../types/core-table-cell-parser-type';
import { CoreTableCellRendererType } from '../../types/core-table-cell-renderer-type';
import { TableCellAlignmentType } from '../../types/table-cell-alignment-type';

export interface RowData {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You can do

export interface RowData extends Dictionary<unknown> {
baseTimestamp: Date | number;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can the timestamp be string too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, Date can handle the string, I have tested this, works fine

custom type DateOrNumber is used as type for celldata as well so I didn't want to repeat Date | number so created this type.

extends Dictionary<unknown> part is fixed!!

[key: string]: unknown;
baseTimestamp: DateOrNumber;
}

type DateOrNumber = Date | number;
@Component({
selector: 'ht-relative-timestamp-table-cell-renderer',
styleUrls: ['./relative-timestamp-table-cell-renderer.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div
class="relative-timestamp"
[htTooltip]="this.value | htDisplayDate: this.dateFormat"
[ngClass]="{ 'first-column': this.isFirstColumn }"
>
{{ this.duration }} ms
</div>
`
})
@TableCellRenderer({
type: CoreTableCellRendererType.RelativeTimestamp,
alignment: TableCellAlignmentType.Left,
parser: CoreTableCellParserType.NoOp
})
export class RelativeTimestampTableCellRendererComponent extends TableCellRendererBase<DateOrNumber> implements OnInit {
public readonly dateFormat: DateFormatOptions = {
mode: DateFormatMode.DateAndTimeWithSeconds
};
public readonly duration: number;
private readonly dateCoercer: DateCoercer = new DateCoercer();

public constructor(
@Inject(TABLE_COLUMN_CONFIG) columnConfig: TableColumnConfig,
@Inject(TABLE_COLUMN_INDEX) index: number,
@Inject(TABLE_DATA_PARSER)
parser: TableCellParserBase<DateOrNumber, DateOrNumber, unknown>,
@Inject(TABLE_CELL_DATA) cellData: DateOrNumber,
@Inject(TABLE_ROW_DATA) rowData: RowData
) {
super(columnConfig, index, parser, cellData, rowData);
this.duration =
(this.dateCoercer.coerce(cellData)?.getTime() ?? NaN) -
(this.dateCoercer.coerce(rowData.baseTimestamp)?.getTime() ?? NaN);
}
}
7 changes: 5 additions & 2 deletions projects/components/src/table/cells/table-cells.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { CodeTableCellRendererComponent } from './data-renderers/code/code-table
import { StringEnumTableCellRendererComponent } from './data-renderers/enum/string-enum-table-cell-renderer.component';
import { IconTableCellRendererComponent } from './data-renderers/icon/icon-table-cell-renderer.component';
import { NumericTableCellRendererComponent } from './data-renderers/numeric/numeric-table-cell-renderer.component';
import { RelativeTimestampTableCellRendererComponent } from './data-renderers/relative-timestamp/relative-timestamp-table-cell-renderer.component';
import { StringArrayTableCellRendererComponent } from './data-renderers/string-array/string-array-table-cell-renderer.component';
import { TableDataCellRendererComponent } from './data-renderers/table-data-cell-renderer.component';
import { TextWithCopyActionTableCellRendererComponent } from './data-renderers/text-with-copy/text-with-copy-table-cell-renderer.component';
Expand Down Expand Up @@ -72,7 +73,8 @@ export const TABLE_CELL_PARSERS = new InjectionToken<unknown[][]>('TABLE_CELL_PA
CodeTableCellRendererComponent,
StringArrayTableCellRendererComponent,
StringEnumTableCellRendererComponent,
TextWithCopyActionTableCellRendererComponent
TextWithCopyActionTableCellRendererComponent,
RelativeTimestampTableCellRendererComponent
],
providers: [
{
Expand All @@ -88,7 +90,8 @@ export const TABLE_CELL_PARSERS = new InjectionToken<unknown[][]>('TABLE_CELL_PA
CodeTableCellRendererComponent,
StringArrayTableCellRendererComponent,
StringEnumTableCellRendererComponent,
TextWithCopyActionTableCellRendererComponent
TextWithCopyActionTableCellRendererComponent,
RelativeTimestampTableCellRendererComponent
],
multi: true
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const enum CoreTableCellRendererType {
Icon = 'icon',
Number = 'number',
RowExpander = 'row-expander',
RelativeTimestamp = 'relative-timestamp',
StringArray = 'string-array',
StringEnum = 'string-enum',
Text = 'text',
Expand Down
1 change: 1 addition & 0 deletions projects/distributed-tracing/src/public-api.ts
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
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;
logEvents?: LogEvent[];
}
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
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'),
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
}));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { Dictionary } from '@hypertrace/common';
import { createModelFactory, SpectatorModel } from '@hypertrace/dashboards/testing';
import { LogEvent } from '@hypertrace/distributed-tracing';
import { GraphQlRequestService } from '@hypertrace/graphql-client';
import { ModelApi } from '@hypertrace/hyperdash';
import { runFakeRxjs } from '@hypertrace/test-utils';
import { mockProvider } from '@ngneat/spectator/jest';
import { of } from 'rxjs';
import { LogDetailDataSourceModel } from './log-detail-data-source.model';

describe('Log Detail data source model', () => {
let spectator!: SpectatorModel<LogDetailDataSourceModel>;
const attributes: Dictionary<unknown> = {
key1: 'value1',
key2: 'value2'
};
const logEvent: LogEvent = {
traceId: 'id1',
attributes: attributes,
spanId: 's-id1',
timestamp: '2021-05-05T00:00:00Z',
summary: 'test log event'
};

const buildModel = createModelFactory({
providers: [
mockProvider(GraphQlRequestService, {
query: jest.fn().mockReturnValue(of({}))
})
]
});
beforeEach(() => {
const mockApi: Partial<ModelApi> = {};
spectator = buildModel(LogDetailDataSourceModel);
spectator.model.logEvent = logEvent;
spectator.model.api = mockApi as ModelApi;
});

test('test attribute data', () => {
runFakeRxjs(({ expectObservable }) => {
expectObservable(spectator.model.getData()).toBe('(x|)', {
x: expect.objectContaining({
key1: 'value1',
key2: 'value2'
})
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { Dictionary } from '@hypertrace/common';
import { Model, ModelProperty, PLAIN_OBJECT_PROPERTY } from '@hypertrace/hyperdash';
import { Observable, of } from 'rxjs';
import { GraphQlDataSourceModel } from '../../../data/graphql/graphql-data-source.model';
import { LogEvent } from '../../waterfall/waterfall/waterfall-chart';

@Model({
type: 'log-detail-data-source'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this page really needs to be a widget - I wouldn't actively avoid it, and if the work is already done it's fine - but for future pages, we really need to get out of the habit of making complex full-screen widgets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense for this to be a widget so that it can go in as a full page list in the trace level view in Phase 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd extend the same argument to those views too though 😉 If I understand the comment/remember the screen, the trace view today is one big widget, and with this it will now have two views - the sequence and the log list, right? When we do that work, I'd make the same argument that the tabs and their contents need not be (but easily could be) dashboards.

Copy link
Contributor

Choose a reason for hiding this comment

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

@itssharmasandeep This is for the tab view right? don't we fetch all the details beforehand and show the tab as a normal component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but since we're using table dashboard, so for child template we need data source, right?
Currently I am not fetching anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think my comment got lost. My original comment was about the table itself - if that needed to be a dashboard (and likewise, everything downstream). The dashboards have really... evolved past how they were intended to be used. Here for example, we could just as easily remove the data source and pass the data directly to the widget - it's not providing any value if it's a custom pass through for a custom widget. (just to clarify, we don't need to change this here, but it's something we should be cognizant of as we build new screens).

As far as this data source goes, can we just reuse StaticDataSource since this one's not doing anything but unwrapping which we could do elsewhere? Or if we keep it, it shouldn't extend GraphQlDataSourceModel because it's not querying GQL.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to aaron's point.

@itssharmasandeep For the sheet/tab view, we don't necessarily need a data source/widget for child template. You can just pass any ng-template in it and it would render. For the page view, we can use the static data source like Aaron described above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shared the details on slack

})
export class LogDetailDataSourceModel extends GraphQlDataSourceModel<Dictionary<unknown>> {
@ModelProperty({
key: 'log-event',
required: true,
type: PLAIN_OBJECT_PROPERTY.type
})
public logEvent?: LogEvent;

public getData(): Observable<Dictionary<unknown>> {
return of(this.logEvent?.attributes ?? {});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
@import 'font';

.content {
@include body-2-regular();
margin-left: 175px;
margin-top: 15px;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { Dictionary, FormattingModule } from '@hypertrace/common';
import { ListViewComponent, LoadAsyncModule } from '@hypertrace/components';
import { mockDashboardWidgetProviders } from '@hypertrace/dashboards/testing';
import { createComponentFactory } from '@ngneat/spectator/jest';
import { MockComponent } from 'ng-mocks';
import { of } from 'rxjs';
import { LogDetailWidgetRendererComponent } from './log-detail-widget-renderer.component';
import { LogDetailWidgetModel } from './log-detail-widget.model';

describe('log detail widget renderer component', () => {
let mockModel: Partial<LogDetailWidgetModel> = {};
const buildComponent = createComponentFactory({
component: LogDetailWidgetRendererComponent,
providers: [],
imports: [FormattingModule, LoadAsyncModule],
declarations: [MockComponent(ListViewComponent)],
shallow: true
});
const attributes: Dictionary<unknown> = {
key1: 1,
key2: 2
};

test('should render list view with provided data', () => {
mockModel = {
getData: jest.fn(() => of(attributes))
};
const spectator = buildComponent({
providers: [...mockDashboardWidgetProviders(mockModel)]
});
expect(spectator.query('.content')).toExist();
expect(spectator.query(ListViewComponent)!.header).toEqual(spectator.component.header);
expect(spectator.query(ListViewComponent)!.records).toEqual([
{
key: 'key1',
value: 1
},
{
key: 'key2',
value: 2
}
]);
});
});
Loading