feat: log records in a new tab #916

itssharmasandeep · 2021-06-09T13:01:40Z

Description

log records in a new tab

Testing

Local testing done!!

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2021-06-09T13:06:09Z

Codecov Report

Merging #916 (f7f9b9f) into main (5de8ca7) will increase coverage by 0.07%.
The diff coverage is 83.83%.

@@            Coverage Diff             @@
##             main     #916      +/-   ##
==========================================
+ Coverage   85.14%   85.21%   +0.07%     
==========================================
  Files         806      812       +6     
  Lines       16644    16714      +70     
  Branches     2098     2003      -95     
==========================================
+ Hits        14171    14243      +72     
+ Misses       2441     2439       -2     
  Partials       32       32

Impacted Files	Coverage Δ
.../pages/trace-detail/trace-detail.page.component.ts	`41.66% <0.00%> (+11.66%)`	⬆️
...ed/components/span-detail/span-detail.component.ts	`100.00% <ø> (ø)`
...pi-trace-detail/api-trace-detail.page.component.ts	`38.46% <0.00%> (+14.65%)`	⬆️
...omponents/log-events/log-events-table.component.ts	`70.00% <42.85%> (-7.78%)`	⬇️
...rc/pages/trace-detail/logs/trace-logs.component.ts	`60.00% <60.00%> (ø)`
.../api-trace-detail/logs/api-trace-logs.component.ts	`60.00% <60.00%> (ø)`
.../trace-detail/sequence/trace-sequence.component.ts	`62.50% <62.50%> (ø)`
...ce-detail/sequence/api-trace-sequence.component.ts	`62.50% <62.50%> (ø)`
.../trace-detail/sequence/trace-sequence.dashboard.ts	`100.00% <100.00%> (ø)`
...src/pages/trace-detail/trace-detail.page.module.ts	`100.00% <100.00%> (ø)`
... and 17 more

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 5de8ca7...f7f9b9f. Read the comment docs.

projects/distributed-tracing/src/pages/trace-detail/sequence/trace-sequence.component.ts

projects/distributed-tracing/src/pages/trace-detail/trace-detail.service.ts

...ted-tracing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.ts

...ability/src/shared/dashboard/data/graphql/waterfall/api-trace-waterfall-data-source.model.ts

projects/distributed-tracing/src/shared/components/log-events/log-events-table.component.ts

projects/distributed-tracing/src/pages/trace-detail/trace-detail.service.test.ts

projects/distributed-tracing/src/pages/trace-detail/trace-detail.service.ts

anandtiwary · 2021-06-09T20:47:44Z

projects/distributed-tracing/src/pages/trace-detail/logs/trace-logs.component.ts

+@Component({
+  changeDetection: ChangeDetectionStrategy.OnPush,
+  template: `
+    <ng-container *ngIf="this.logEvents$ | async as logEvents">


If we are fetching from graphql then we should use *htLoadAsync so that we can show the loader icon

Actually, for this
Data is already being fetched before this, on the page level. and here it will be only used as a static data observable (shareReplay)

okay. I hope that doesn't change :)

It is better to use loadAsync. If in case fetchLogEvents starts returning a new observable, we would have to remember to update this template as well.

projects/distributed-tracing/src/pages/trace-detail/trace-detail.page.component.ts

...ted-tracing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.ts

projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts

projects/observability/src/pages/api-trace-detail/api-trace-detail.service.ts

anandtiwary · 2021-06-16T05:36:34Z

...racing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.test.ts

@@ -221,7 +226,8 @@ describe('Trace Waterfall data source model', () => {
              displaySpanName: 'Span Name 2',
              serviceName: 'Service Name 2',
              type: SpanType.Exit,
-              spanTags: {}
+              spanTags: {},
+              logEvents: []


We could add properties so that the test becomes relevant :)

anandtiwary

Looks good. Please test the two pages in ht-ui and t-ui before merging.

aaron-steinfeld · 2021-06-16T12:43:40Z

Some suggestions based on offline conversation (haven't reviewed most of PR, will leave that to others): #935

anandtiwary · 2021-06-16T16:20:28Z

projects/distributed-tracing/src/pages/trace-detail/logs/trace-logs.component.ts

+    </ng-container>
+  `
+})
+export class TraceLogsComponent {


So this is not a dash page. It works. But I am not sure if we should be diverge from the dashboard pattern.

@aaron-steinfeld thoughts?

IMO, we should try to reign dashboards back in to pages that look like dashboards. If we later need to embed this as a widget in another page, we could widgetize it for sharing at that point - but no need to build these custom single-page, single-purpose widgets.

github-actions · 2021-06-17T17:32:12Z

Unit Test Results

    4 files ±0 259 suites +1 13m 57s ⏱️ -28s
936 tests +7 936 ✔️ +7 0 💤 ±0 0 ❌ ±0
942 runs +7 942 ✔️ +7 0 💤 ±0 0 ❌ ±0

Results for commit 3d7da37. ± Comparison against base commit 5de8ca7.

feat: log records in a new tab

0bea80a

itssharmasandeep requested a review from a team as a code owner June 9, 2021 13:01

feat: log records in a new tab

dbe6bf2

This comment has been minimized.

Sign in to view

arjunlalb reviewed Jun 9, 2021

View reviewed changes

projects/distributed-tracing/src/shared/components/log-events/log-events-table.component.ts Show resolved Hide resolved

arjunlalb reviewed Jun 9, 2021

View reviewed changes

anandtiwary reviewed Jun 9, 2021

View reviewed changes

projects/distributed-tracing/src/pages/trace-detail/trace-detail.page.component.ts Show resolved Hide resolved

anandtiwary reviewed Jun 9, 2021

View reviewed changes

projects/distributed-tracing/src/pages/trace-detail/trace-detail.page.component.ts Show resolved Hide resolved

anandtiwary reviewed Jun 9, 2021

View reviewed changes

...ted-tracing/src/shared/dashboard/data/graphql/waterfall/trace-waterfall-data-source.model.ts Outdated Show resolved Hide resolved

anandtiwary reviewed Jun 9, 2021

View reviewed changes

projects/observability/src/pages/api-trace-detail/api-trace-detail.page.component.ts Show resolved Hide resolved

anandtiwary reviewed Jun 9, 2021

View reviewed changes

projects/observability/src/pages/api-trace-detail/api-trace-detail.service.ts Outdated Show resolved Hide resolved

itssharmasandeep added 3 commits June 11, 2021 19:24

Merge remote-tracking branch origin/main into log-tab

4242474

Merge remote-tracking branch 'origin/main' into log-tab

04900e5

fix: addressing review comments

a396411

This comment has been minimized.

Sign in to view

itssharmasandeep added 3 commits June 15, 2021 12:16

fix: adding log events service

e0b6ffb

fix: test cases

8c757ba

fix: test cases and lint errors

6d8d2c6

This comment has been minimized.

Sign in to view

anandtiwary reviewed Jun 16, 2021

View reviewed changes

anandtiwary previously approved these changes Jun 16, 2021

View reviewed changes

itssharmasandeep added 2 commits June 16, 2021 14:37

Merge remote-tracking branch 'origin/main' into log-tab

8fed170

fix: style fix

e055fb4

itssharmasandeep dismissed anandtiwary’s stale review via e055fb4 June 16, 2021 09:47

This comment has been minimized.

Sign in to view

itssharmasandeep and others added 2 commits June 16, 2021 18:19

Merge remote-tracking branch 'origin/main' into log-tab

5255c03

chore: suggested-log-tab-changes (#935)

ac2fec0

This comment has been minimized.

Sign in to view

anandtiwary reviewed Jun 16, 2021

View reviewed changes

anandtiwary previously approved these changes Jun 16, 2021

View reviewed changes

itssharmasandeep added 2 commits June 17, 2021 17:45

Merge remote-tracking branch 'origin/main' into log-tab

748f0a1

fix: adding exports

f7f9b9f

itssharmasandeep dismissed anandtiwary’s stale review via f7f9b9f June 17, 2021 12:19

This comment has been minimized.

Sign in to view

anandtiwary approved these changes Jun 17, 2021

View reviewed changes

itssharmasandeep merged commit 3d7da37 into main Jun 17, 2021

itssharmasandeep deleted the log-tab branch June 17, 2021 17:27

feat: log records in a new tab #916

feat: log records in a new tab #916

Uh oh!

Conversation

itssharmasandeep commented Jun 9, 2021

Description

Testing

Checklist:

Uh oh!

This comment has been minimized.

codecov bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anandtiwary Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

itssharmasandeep Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

anandtiwary Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

anandtiwary Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

anandtiwary Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

anandtiwary left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

aaron-steinfeld commented Jun 16, 2021

Uh oh!

This comment has been minimized.

anandtiwary Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions bot commented Jun 17, 2021

Unit Test Results

Uh oh!

Uh oh!

codecov bot commented Jun 9, 2021 •

edited

Loading