feat: fetching log records and showing log icon #817
Conversation
Codecov Report
@@ Coverage Diff @@
## main #817 +/- ##
========================================
Coverage 85.44% 85.44%
========================================
Files 792 792
Lines 16212 16221 +9
Branches 2073 1930 -143
========================================
+ Hits 13852 13860 +8
Misses 2329 2329
- Partials 31 32 +1
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
...ects/distributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.ts
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -12,4 +13,6 @@ export interface SpanData { | |||
| tags: Dictionary<unknown>; | |||
| requestUrl: string; | |||
| exitCallsBreakup?: Dictionary<string>; | |||
| startTime?: number; | |||
There was a problem hiding this comment.
Surprised startTime isn't required. Is there a reason some of these properties optional?
There was a problem hiding this comment.
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.
| @@ -23,6 +23,7 @@ export interface WaterfallData { | |||
| responseBody?: string; | |||
| tags: Dictionary<unknown>; | |||
| errorCount: number; | |||
| logEvents?: LogEvent[]; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, this can be the case.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Added extra condition to handle that.
So it is not optional anymore.
This reverts commit 8886ae2.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -60,6 +60,14 @@ export class TraceWaterfallDataSourceModel extends GraphQlDataSourceModel<Waterf | |||
| this.specificationBuilder.attributeSpecificationForKey('errorCount') | |||
| ]; | |||
|
|
|||
| protected readonly logEventSpecifications: Specification[] = [ | |||
| this.specificationBuilder.attributeSpecificationForKey('traceId'), | |||
There was a problem hiding this comment.
nit: do we actually need the spanid and trace id in these? isn't it already available in the span parent?
There was a problem hiding this comment.
yeah, we don't need that actually.
thank you for pointing that out.
| @@ -37,3 +38,12 @@ export interface WaterfallChartState { | |||
| children: WaterfallDataNode[]; | |||
| expanded: boolean; | |||
| } | |||
|
|
|||
| export interface LogEvent { | |||
| [key: string]: unknown; | |||
There was a problem hiding this comment.
what unknown fields does this have? would anything break if you removed this?
There was a problem hiding this comment.
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!!
| @@ -23,6 +23,7 @@ export interface WaterfallData { | |||
| responseBody?: string; | |||
| tags: Dictionary<unknown>; | |||
| errorCount: number; | |||
| logEvents?: LogEvent[]; | |||
There was a problem hiding this comment.
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).
| @@ -12,4 +13,6 @@ export interface SpanData { | |||
| tags: Dictionary<unknown>; | |||
| requestUrl: string; | |||
| exitCallsBreakup?: Dictionary<string>; | |||
| startTime: number; | |||
There was a problem hiding this comment.
We hold timestamps as dates generally - converting them as soon as feasible. In this PR, it looks like we're adding a few different timestamp fields - numbers in one spot, string in another. Can those be converted to dates up front?
Description
Fetching log records and showing log icon
Testing
Local testing done.
Checklist:
Example
