Skip to content

Conversation

itssharmasandeep
Copy link
Contributor

Description

Removing Filtering for exit calls cell in trace table

Testing

Local testing done

Checklist:

  • My changes generate no new warnings

@itssharmasandeep itssharmasandeep requested a review from a team as a code owner April 15, 2021 08:17
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #772 (8945f86) into main (4d53214) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #772   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files         789      789           
  Lines       16145    16145           
  Branches     2060     2060           
=======================================
  Hits        13785    13785           
  Misses       2329     2329           
  Partials       31       31           
Impacted Files Coverage Δ
...apis/api-detail/traces/api-trace-list.dashboard.ts 100.00% <ø> (ø)
...vice-detail/traces/service-trace-list.dashboard.ts 100.00% <ø> (ø)
...-calls/exit-calls-table-cell-renderer.component.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d53214...8945f86. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

})
export class ExitCallsTableCellRendererComponent extends TableCellRendererBase<CellData, Trace> implements OnInit {
public readonly apiCalleeNameCount: string[][];
public readonly apiCalleeNameEntires: [string, string][];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public readonly apiCalleeNameEntires: [string, string][];
public readonly apiCalleeNameEntries: [string, string][];

(and references)

public readonly apiExitCalls: number;
public readonly maxShowApiCalleeNameCount: number = 10;
public readonly totalCountOfDifferentApiCallee!: number;
public readonly MAX_API_CALLEE_TO_SHOW: number = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

With a const like this, we should make it static (and lift it above the instance vars)

Copy link
Contributor

Choose a reason for hiding this comment

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

also, please rename to MAX_API_CALLEES_TO_SHOW

public readonly maxShowApiCalleeNameCount: number = 10;
public readonly totalCountOfDifferentApiCallee!: number;
public readonly MAX_API_CALLEE_TO_SHOW: number = 10;
public readonly uniqueApiCallee: number;
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 recommend uniqueApiCalleeCount

public readonly apiExitCalls: number;
public readonly maxShowApiCalleeNameCount: number = 10;
public readonly totalCountOfDifferentApiCallee!: number;
public readonly MAX_API_CALLEE_TO_SHOW: number = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

also, please rename to MAX_API_CALLEES_TO_SHOW

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@itssharmasandeep itssharmasandeep merged commit f0295d8 into main Apr 15, 2021
@itssharmasandeep itssharmasandeep deleted the exit-calls branch April 15, 2021 13:05
@github-actions
Copy link

Unit Test Results

    4 files  ±0  248 suites  ±0   15m 52s ⏱️ + 2m 44s
889 tests ±0  889 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
895 runs  ±0  895 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f0295d8. ± Comparison against base commit 4d53214.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants