feat: highlight error span on partial or full error #773

itssharmasandeep · 2021-04-15T12:07:37Z

Description

To highlight error span or traces on partial or full error

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-04-15T12:11:08Z

Codecov Report

Merging #773 (c6898e5) into main (782a58f) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
- Coverage   85.42%   85.41%   -0.01%     
==========================================
  Files         792      792              
  Lines       16161    16163       +2     
  Branches     2060     2061       +1     
==========================================
+ Hits        13805    13806       +1     
- Misses       2325     2326       +1     
  Partials       31       31

Impacted Files	Coverage Δ
...hql/waterfall/trace-waterfall-data-source.model.ts	`96.66% <ø> (ø)`
...waterfall/api-trace-waterfall-data-source.model.ts	`96.55% <ø> (ø)`
...ets/waterfall/waterfall/waterfall-chart.service.ts	`95.16% <66.66%> (-1.51%)`	⬇️
...an-name/span-name-table-cell-renderer.component.ts	`100.00% <100.00%> (ø)`
...gets/waterfall/waterfall/waterfall-chart.module.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 782a58f...c6898e5. Read the comment docs.

aaron-steinfeld · 2021-04-15T18:33:03Z

Pausing this one based on mock discussions around how to make it clearer when a downstream error vs an actual otel-defined error.

kotharironak · 2021-04-19T12:32:47Z

projects/observability/src/pages/explorer/explorer-dashboard-builder.ts

          },
          {
            type: 'table-widget-column',
            title: 'Exit Calls',


I think the name of an attribute is apiTraceErrorSpanCount in API_TRACE scop
cc: @JBAhire

While using the attribute named errorCount, I am getting the data but not getting any data while using apiTraceErrorSpanCount

That's weird because I can see data in Pinot as well as filters on explorer and service screen. Maybe the data you are trying with doesn't have any error spans in API trace?

errorCount is a different thing - it will be 1 if the api trace is overall in error, 0 otherwise (used for calculating error metrics)

It does work with apiTraceErrorSpanCount now.

referennce: #786

aaron-steinfeld · 2021-04-19T18:12:02Z

...stributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.module.ts

+    IconModule
  ],
-  exports: [WaterfallChartComponent]
+  exports: [WaterfallChartComponent, SpanNameTableCellRendererComponent]


This shouldn't need to be exported since we don't directly reference renderers - did you run into an issue?

I guess, It was a mistake.
Fixed

aaron-steinfeld · 2021-04-19T18:16:49Z

...tributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.service.ts

          serviceName: datum.serviceName,
-          protocolName: datum.protocolName
+          protocolName: datum.protocolName,
+          error: datum.tags?.error as boolean


isn't the error state a top level attribute? should we be fetching it directly rather than through the tag map (which means that it would be populated potentially via enrichment, since there's more than one way to mark an error).

I don't think it is a top level attribute. Whenever I console log the data, I did not see any error attribute but there is one in the tags.
May be I am missing something.
cc: @kotharironak

The top level attributes need to be added to the request, too. See the various waterfall data source models (for example from trace-waterfall-data-source.model.ts:

protected readonly spanSpecifications: Specification[] = [ this.specificationBuilder.attributeSpecificationForKey('displaySpanName'), this.specificationBuilder.attributeSpecificationForKey('duration'), this.specificationBuilder.attributeSpecificationForKey('endTime'), this.specificationBuilder.attributeSpecificationForKey('parentSpanId'), this.specificationBuilder.attributeSpecificationForKey('serviceName'), this.specificationBuilder.attributeSpecificationForKey('spanTags'), this.specificationBuilder.attributeSpecificationForKey('startTime'), this.specificationBuilder.attributeSpecificationForKey('type'), this.specificationBuilder.attributeSpecificationForKey('traceId') ];

@itssharmasandeep we should be using EVENT. errorCount > 0 to indicate error. This value of this attribute is based on tags that indicate error at span level.
https://github.com/hypertrace/config-bootstrapper/blob/main/config-bootstrapper/src/main/resources/configs/config-bootstrapper/attribute-service/hypertrace/hypertrace_span_attributes.conf#L62

Yeah, I am using the same now.
updated the PR.

aaron-steinfeld · 2021-04-19T18:18:10Z

...tributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.service.ts

-        color = originalColors[uniqueServiceIndex % originalColors.length];
-        colorMap.set(node.serviceName, color);
-        uniqueServiceIndex++;
+        if (colorMap.has(node.serviceName)) {


extra nest (use else if)

projects/observability/src/pages/apis/api-detail/traces/api-trace-list.dashboard.ts

aaron-steinfeld · 2021-04-19T18:19:20Z

projects/observability/src/pages/apis/api-detail/traces/api-trace-list.dashboard.ts

+      {
+        type: 'table-widget-column',
+        title: 'Errors',
+        width: '180px',


This is just a number right? 180px feels a bit excessive

Please refer to: #786

aaron-steinfeld · 2021-04-19T18:20:46Z

projects/observability/src/pages/explorer/explorer-dashboard-builder.ts

          },
          {
            type: 'table-widget-column',
            title: 'Exit Calls',


errorCount is a different thing - it will be 1 if the api trace is overall in error, 0 otherwise (used for calculating error metrics)

itssharmasandeep · 2021-04-20T10:12:24Z

I have update the PR. So it doesn't contain the changes related to adding error column in the trace lists/tables.
For that change please refer to #786

@kotharironak @aaron-steinfeld

aaron-steinfeld · 2021-04-20T16:20:40Z

Thanks for splitting em up

… span-error

anandtiwary · 2021-04-21T17:18:46Z

...tributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.service.ts

      let color;

-      if (colorMap.has(node.serviceName)) {
+      if (node.$$spanName.error) {


nit: the color logic can be isolated to its own method now (since it has gotten bigger).

I guess, we don't need this as of now.
It is not that big and we also need to handle some local vars(the way code is written currently)

anandtiwary · 2021-04-21T17:20:03Z

...tributed-tracing/src/shared/dashboard/widgets/waterfall/waterfall/waterfall-chart.service.ts

          serviceName: datum.serviceName,
-          protocolName: datum.protocolName
+          protocolName: datum.protocolName,
+          error: datum.errorCount > 0


nit: hasError would convey it is boolean and not an error string

Yeah,
I have made the changes

arjunlalb

LGTM

github-actions · 2021-04-22T07:09:52Z

Unit Test Results

    4 files ±0 250 suites ±0 15m 21s ⏱️ + 1m 39s
897 tests ±0 897 ✔️ ±0 0 💤 ±0 0 ❌ ±0
903 runs ±0 903 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 300ed2e. ± Comparison against base commit 782a58f.

feat: highlight error span or traces on partial or full error

b1ce950

itssharmasandeep requested a review from a team as a code owner April 15, 2021 12:07

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'origin/main' into span-error

a407cf3

itssharmasandeep requested review from JBAhire, findingrish and kotharironak April 16, 2021 05:17

itssharmasandeep added 3 commits April 16, 2021 18:57

Merge remote-tracking branch 'origin/main' into span-error

254a519

feat: adding errors column to trace lists

0593e3f

refactor: removing unwanted code

4f83aaa

This comment has been minimized.

Sign in to view

kotharironak reviewed Apr 19, 2021

View reviewed changes

Merge branch 'main' into span-error

bc46627

This comment has been minimized.

Sign in to view

aaron-steinfeld reviewed Apr 19, 2021

View reviewed changes

Merge branch 'main' into span-error

db30235

This comment has been minimized.

Sign in to view

fix: addressing review comments

9ba8708

This comment has been minimized.

Sign in to view

itssharmasandeep changed the title ~~feat: highlight error span or traces on partial or full error~~ feat: highlight error span on partial or full error Apr 20, 2021

Merge branch 'main' into span-error

fd9df26

This comment has been minimized.

Sign in to view

itssharmasandeep added 2 commits April 21, 2021 12:54

fix: using errorcount attribute

150943f

Merge branch 'span-error' of github.com:hypertrace/hypertrace-ui into…

6b75bdb

… span-error

itssharmasandeep removed the request for review from findingrish April 21, 2021 09:46

itssharmasandeep removed the request for review from JBAhire April 21, 2021 09:46

This comment has been minimized.

Sign in to view

anandtiwary reviewed Apr 21, 2021

View reviewed changes

anandtiwary previously approved these changes Apr 21, 2021

View reviewed changes

Merge branch 'main' into span-error

c3f9b0a

This comment has been minimized.

Sign in to view

fix: test cases

093f57f

itssharmasandeep dismissed anandtiwary’s stale review via 093f57f April 22, 2021 05:42

This comment has been minimized.

Sign in to view

refactor: addressing review comments

c6898e5

This comment has been minimized.

Sign in to view

arjunlalb approved these changes Apr 22, 2021

View reviewed changes

anandtiwary approved these changes Apr 22, 2021

View reviewed changes

itssharmasandeep merged commit 300ed2e into main Apr 22, 2021

itssharmasandeep deleted the span-error branch April 22, 2021 07:05

feat: highlight error span on partial or full error #773

feat: highlight error span on partial or full error #773

Uh oh!

Conversation

itssharmasandeep commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

codecov bot commented Apr 15, 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.

aaron-steinfeld commented Apr 15, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JBAhire Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

itssharmasandeep commented Apr 20, 2021

Uh oh!

aaron-steinfeld commented Apr 20, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

arjunlalb left a comment

Choose a reason for hiding this comment

itssharmasandeep commented Apr 15, 2021 •

edited

Loading

codecov bot commented Apr 15, 2021 •

edited

Loading

JBAhire Apr 19, 2021 •

edited

Loading