feat: relative timestamp table cell renderer and log events table in sheet #818
Conversation
Codecov Report
@@ Coverage Diff @@
## main #818 +/- ##
==========================================
+ Coverage 85.44% 85.46% +0.02%
==========================================
Files 793 796 +3
Lines 16252 16299 +47
Branches 1932 1937 +5
==========================================
+ Hits 13886 13930 +44
- Misses 2334 2337 +3
Partials 32 32
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| import { ObservabilityTableCellType } from '../../observability-table-cell-type'; | ||
|
|
||
| @Component({ | ||
| selector: 'ht-log-timestamp-table-cell-renderer', |
There was a problem hiding this comment.
Can we decouple this from logs? This is just a general relative timestamp renderer, right?
There was a problem hiding this comment.
Done that!!
| import { LogEvent } from '../../waterfall/waterfall/waterfall-chart'; | ||
|
|
||
| @Model({ | ||
| type: 'log-detail-data-source' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@itssharmasandeep This is for the tab view right? don't we fetch all the details beforehand and show the tab as a normal component?
There was a problem hiding this comment.
Yeah, but since we're using table dashboard, so for child template we need data source, right?
Currently I am not fetching anything.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
Shared the details on slack
| } | ||
|
|
||
| public getLogEventAttributeRecords(attributes: Dictionary<unknown>): ListViewRecord[] { | ||
| if (Boolean(attributes)) { |
| import { ObservabilityTableCellType } from '../../observability-table-cell-type'; | ||
|
|
||
| @Component({ | ||
| selector: 'ht-log-timestamp-table-cell-renderer', |
| this.spanStartTime = rowData.spanStartTime as number; | ||
| this.timestamp = rowData.timestamp; | ||
| this.readableDateTime = | ||
| this.timestamp |
There was a problem hiding this comment.
is there a cleaner way to do this? whats the goal here? Why do we have a time string as input here?
There was a problem hiding this comment.
+1 - please use native/our existing utils like date coercer and TImeDuration. We never should be working with the strings directly. This one, for example we could take the difference and use our duration class.
There was a problem hiding this comment.
So currently we're getting a timestamp string which is in nano seconds for example 2021-05-04T12:13:55.543456T
So all this logic is done to show cell value and tooltip in an expected format.
There was a problem hiding this comment.
Nanos shouldn't be relevant at this level - that's why we use the ISO format, so the UI is agnostic to the unit. The timestamp should render the same as all our other timestamps, right? Using the date coercer, then the display date pipe. The new part is the relative offset between the two timestamps (log event time and span start time), which would require taking the difference between the two dates (which were both resolved via date coercer) and using our duration pipe (I'm not 100% sure that duration pipe is up to date for these requirements, but conceptually at least, that's what we should be going for)
There was a problem hiding this comment.
I have updated the new look for this in description.
Please do have a look!!
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.
This comment has been minimized.
This comment has been minimized.
| RowData | ||
| } from './relative-timestamp-table-cell-renderer.component'; | ||
|
|
||
| describe('log timestamp table cell renderer component', () => { |
There was a problem hiding this comment.
nit: relative timestamp
| import { CoreTableCellRendererType } from '../../types/core-table-cell-renderer-type'; | ||
| import { TableCellAlignmentType } from '../../types/table-cell-alignment-type'; | ||
|
|
||
| export interface RowData { |
There was a problem hiding this comment.
nit: You can do
export interface RowData extends Dictionary<unknown> {
baseTimestamp: Date | number;
}
There was a problem hiding this comment.
Also, can the timestamp be string too?
There was a problem hiding this comment.
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!!
| import { LogEvent } from '../../waterfall/waterfall/waterfall-chart'; | ||
|
|
||
| @Model({ | ||
| type: 'log-detail-data-source' |
There was a problem hiding this comment.
@itssharmasandeep This is for the tab view right? don't we fetch all the details beforehand and show the tab as a normal component?
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.
* feat: log records in the span sheet
This comment has been minimized.
This comment has been minimized.
projects/distributed-tracing/src/shared/components/log-events/log-events-table.component.ts
Show resolved
Hide resolved
| import { Observable, of } from 'rxjs'; | ||
|
|
||
| export const enum LogEventsTableViewType { | ||
| Sheet = 'sheet', |
There was a problem hiding this comment.
We do need better name for this. Umm. How about Condensed for the sheet and Detailed for the page? Any other suggestions anyone?
There was a problem hiding this comment.
Yeah, these names look good to me, Changed!!
| if (!isEmpty(attributes)) { | ||
| return Object.entries(attributes).map((attribute: [string, unknown]) => ({ | ||
| key: attribute[0], | ||
| value: attribute[1] as string | number |
There was a problem hiding this comment.
So we do know the type of value -> string | number. Why don't we update the type of attributes and logEvents to reflect this?
@Input()
public logEvents: Dictionary<string | number>[] = [];
There was a problem hiding this comment.
Yeah, made the changes.
I also don't know why I did it like this.
There was a problem hiding this comment.
Still don't see this change though
|
|
||
| public getLogEventAttributeRecords(attributes: Dictionary<unknown>): ListViewRecord[] { | ||
| if (!isEmpty(attributes)) { | ||
| return Object.entries(attributes).map((attribute: [string, unknown]) => ({ |
There was a problem hiding this comment.
you can also do
map( ([key, value]) => ({key: key, value: value}))
There was a problem hiding this comment.
Fixed, thanks for suggestion!!
There was a problem hiding this comment.
Still don't see this change though
This comment has been minimized.
This comment has been minimized.
| this.columnConfigs = this.getTableColumnConfigs(); | ||
| } | ||
|
|
||
| public getLogEventAttributeRecords(attributes: Dictionary<unknown>): ListViewRecord[] { |
|
|
||
| private buildDataSource(): void { | ||
| this.dataSource = { | ||
| getData: (): Observable<TableDataResponse<TableRow>> => |
There was a problem hiding this comment.
nit: we can define the type of TableRow returned from this method for clarity.
itssharmasandeep
changed the title
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.
itssharmasandeep
changed the title
Description
Log detail widget and log timestamp table cell renderer components
Testing
Local testing done
Checklist: