feat: show exit calls break up on hover on exit calls cell in trace list #756
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| @import 'color-palette'; | ||
| @import 'font'; | ||
|
|
||
| .exit-calls-count { | ||
| @include body-1-regular($gray-7); | ||
| } | ||
|
|
||
| .api-callee-name-count { | ||
| @include body-small($gray-3); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| padding: 2px; | ||
|
|
||
| .api-callee-name { | ||
| @include ellipsis-overflow(); | ||
| max-width: 200px; | ||
| } | ||
|
|
||
| .api-callee-count { | ||
| color: white; | ||
| margin-left: 50px; | ||
| } | ||
| } | ||
|
|
||
| .remaining-api-callee { | ||
| @include body-small($gray-3); | ||
| padding: 2px; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import { | ||
| tableCellDataProvider, | ||
| TableCellNoOpParser, | ||
| tableCellProviders, | ||
| TooltipDirective | ||
| } from '@hypertrace/components'; | ||
| import { createComponentFactory } from '@ngneat/spectator/jest'; | ||
| import { MockComponent } from 'ng-mocks'; | ||
| import { ExitCallsTableCellRendererComponent } from './exit-calls-table-cell-renderer.component'; | ||
|
|
||
| describe('Exit Calls table cell renderer component', () => { | ||
| const buildComponent = createComponentFactory({ | ||
| component: ExitCallsTableCellRendererComponent, | ||
| providers: [ | ||
| tableCellProviders( | ||
| { | ||
| id: 'test' | ||
| }, | ||
| new TableCellNoOpParser(undefined!) | ||
| ) | ||
| ], | ||
| declarations: [MockComponent(TooltipDirective)], | ||
| shallow: true | ||
| }); | ||
|
|
||
| test('testing component properties', () => { | ||
| const value = { | ||
| key1: '1', | ||
| key2: '2' | ||
| }; | ||
| const spectator = buildComponent({ | ||
| providers: [tableCellDataProvider({ value: [3, value] })] | ||
| }); | ||
|
|
||
| expect(spectator.queryAll('.exit-calls-count')[0]).toContainText('3'); | ||
| expect(spectator.component.apiCalleeNameCount).toMatchObject([ | ||
| ['key1', '1'], | ||
| ['key2', '2'] | ||
| ]); | ||
| expect(spectator.component.totalCountOfDifferentApiCallee).toBe(2); | ||
| expect(spectator.component.apiExitCalls).toBe(3); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { ChangeDetectionStrategy, Component, Inject, OnInit } from '@angular/core'; | ||
| import { Dictionary } from '@hypertrace/common'; | ||
| import { | ||
| CoreTableCellParserType, | ||
| TableCellAlignmentType, | ||
| TableCellParserBase, | ||
| TableCellRenderer, | ||
| TableCellRendererBase, | ||
| TableColumnConfig, | ||
| TABLE_CELL_DATA, | ||
| TABLE_COLUMN_CONFIG, | ||
| TABLE_COLUMN_INDEX, | ||
| TABLE_DATA_PARSER, | ||
| TABLE_ROW_DATA | ||
| } from '@hypertrace/components'; | ||
| import { Trace } from '@hypertrace/distributed-tracing'; | ||
| import { ObservabilityTableCellType } from '../../observability-table-cell-type'; | ||
|
|
||
| interface CellData { | ||
|
There was a problem hiding this comment. Wouldn't the cell data be an array of There was a problem hiding this comment. the query handlers append units on to everything so instead of that array, it's wrapped in an object with units like There was a problem hiding this comment. Did this change recently? I remember using composite spec where I have used value as array directly(let me find it). And regardless, imo the specification should reflect this object structure. There was a problem hiding this comment. Possibly, table is calling parser's There was a problem hiding this comment. This does seem a bit suspicious - the wrapping object should only be there if units are defined, which they shouldn't be for a composite spec. |
||
| units: number; | ||
| value: [number, Dictionary<string>]; | ||
| } | ||
| @Component({ | ||
| selector: 'ht-exit-calls-table-cell-renderer', | ||
| styleUrls: ['./exit-calls-table-cell-renderer.component.scss'], | ||
| changeDetection: ChangeDetectionStrategy.OnPush, | ||
| template: ` | ||
| <div class="exit-calls-cell" [htTooltip]="exitCallsTooltip"> | ||
| <span class="exit-calls-count">{{ this.apiExitCalls }}</span> | ||
| <ng-template #exitCallsTooltip> | ||
| <ng-container *ngIf="this.apiExitCalls > 0"> | ||
|
There was a problem hiding this comment. nit: This is an if-else use case. You can clean this up like below. There are many examples in the codebase. |
||
| <div *ngFor="let item of this.apiCalleeNameCount" class="api-callee-name-count"> | ||
| <span class="api-callee-name">{{ item[0] }}</span> | ||
| <span class="api-callee-count">{{ item[1] }}</span> | ||
| </div> | ||
| <div | ||
| *ngIf="this.totalCountOfDifferentApiCallee > this.maxShowApiCalleeNameCount" | ||
| class="remaining-api-callee" | ||
| > | ||
| and {{ this.totalCountOfDifferentApiCallee - this.maxShowApiCalleeNameCount }} more | ||
| </div> | ||
| </ng-container> | ||
| <ng-container *ngIf="this.apiExitCalls <= 0" class="no-exit-calls">No exit calls</ng-container> | ||
| </ng-template> | ||
| </div> | ||
| ` | ||
| }) | ||
| @TableCellRenderer({ | ||
| type: ObservabilityTableCellType.ExitCalls, | ||
| alignment: TableCellAlignmentType.Left, | ||
| parser: CoreTableCellParserType.NoOp | ||
| }) | ||
| export class ExitCallsTableCellRendererComponent extends TableCellRendererBase<CellData, Trace> implements OnInit { | ||
| public readonly apiCalleeNameCount: string[][]; | ||
| public readonly apiExitCalls: number; | ||
| public readonly maxShowApiCalleeNameCount: number = 10; | ||
| public readonly totalCountOfDifferentApiCallee!: number; | ||
|
|
||
| public constructor( | ||
| @Inject(TABLE_COLUMN_CONFIG) columnConfig: TableColumnConfig, | ||
| @Inject(TABLE_COLUMN_INDEX) index: number, | ||
| @Inject(TABLE_DATA_PARSER) | ||
| parser: TableCellParserBase<CellData, Trace, unknown>, | ||
| @Inject(TABLE_CELL_DATA) cellData: CellData, | ||
| @Inject(TABLE_ROW_DATA) rowData: Trace | ||
| ) { | ||
| super(columnConfig, index, parser, cellData, rowData); | ||
| const apiCalleeNameCount: string[][] = Object.entries(cellData.value[1]); | ||
|
There was a problem hiding this comment.
can we call it
There was a problem hiding this comment. Agree naming is confusing/type suggestions. Line 71 is meant to limit the amount of pairs to show in the tooltip, but again naming would help clarify ( There was a problem hiding this comment. Let's also rename |
||
| this.totalCountOfDifferentApiCallee = apiCalleeNameCount.length; | ||
| this.apiCalleeNameCount = apiCalleeNameCount.slice(0, this.maxShowApiCalleeNameCount); | ||
| this.apiExitCalls = cellData.value[0]; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,28 @@ | ||
| import { CommonModule } from '@angular/common'; | ||
| import { NgModule } from '@angular/core'; | ||
| import { TableModule, TooltipModule } from '@hypertrace/components'; | ||
| import { BackendIconTableCellParser } from './data-cell/backend-icon/backend-icon-table-cell-parser'; | ||
| import { BackendIconTableCellRendererComponent } from './data-cell/backend-icon/backend-icon-table-cell-renderer.component'; | ||
| import { BackendIconTableCellRendererModule } from './data-cell/backend-icon/backend-icon-table-cell-renderer.module'; | ||
| import { EntityTableCellParser } from './data-cell/entity/entity-table-cell-parser'; | ||
| import { EntityTableCellRendererComponent } from './data-cell/entity/entity-table-cell-renderer.component'; | ||
| import { EntityTableCellRendererModule } from './data-cell/entity/entity-table-cell-renderer.module'; | ||
| import { ExitCallsTableCellRendererComponent } from './data-cell/exit-calls/exit-calls-table-cell-renderer.component'; | ||
|
|
||
| @NgModule({ | ||
| imports: [ | ||
| CommonModule, | ||
| TableModule.withCellParsers([EntityTableCellParser, BackendIconTableCellParser]), | ||
| TableModule.withCellRenderers([ | ||
| EntityTableCellRendererComponent, | ||
| BackendIconTableCellRendererComponent, | ||
| ExitCallsTableCellRendererComponent | ||
| ]), | ||
| EntityTableCellRendererModule, | ||
| BackendIconTableCellRendererModule, | ||
| TooltipModule | ||
| ], | ||
| declarations: [ExitCallsTableCellRendererComponent], | ||
| exports: [ExitCallsTableCellRendererComponent] | ||
| }) | ||
| export class ObservabilityTableCellRendererModule {} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| export const enum ObservabilityTableCellType { | ||
| Entity = 'entity', | ||
| BackendIcon = 'backend-icon', | ||
| ExitCalls = 'exit-calls' | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test has value, but what would be even better would be to verify that this data renders - for example by querying for the rendered spans and checking their content (e.g.
expect(spectator.queryAll('.api-callee-name')[0]).toContainText('key1');)That checks both how the data is processed but also that the template is correctly binding what we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this change I can make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, discussed offline - because this is a template that's not rendered in the scope of this component, it's ugly to test and not worth it.