Fix error details not being logged in the tracing's span event #543

filipewl · 2023-09-14T17:07:14Z

What is the purpose of this pull request?

Fix how the field "error" of an "error" span event is not displaying the error's details.

Before	After

What problem is this solving?

It appears this "error" field is categorized as a boolean type, so the expected error details isn't being used. By renaming this field to "error.details", we expect to have it displayed on Honeycomb.

How should this be manually tested?

Visit https://filipelimaxtracing--storecomponents.myvtex.com/ where I have yarn-linked this branch to a vtex-linked pages-graphql and render-server apps.
To avoid being sampled, use a browser extension to include the header jaeger-debug-id=ANY_ID_YOU_WANT_USE_TO_QUERY_HONEYCOMB.
Navigate through the store to send some traces to Honeycomb.
In Honeycomb's Query page, add a WHERE filter to jaeger-debug-id = ANY_ID_YOU_WANT_USE_TO_QUERY_HONEYCOMB to find your traces.

Example of a trace with error span. Check the "Span events" tab on the side bar and expand the "error" event.

Types of changes

Bug fix (a non-breaking change which fixes an issue)
New feature (a non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Requires change to documentation, which has been updated accordingly.

filipewl · 2023-09-14T17:12:38Z

src/tracing/errorReporting/ErrorReport.ts

@@ -61,7 +61,7 @@ export class ErrorReport extends ErrorReportBase {
    }

    const serializableError = this.toObject()


toObject returns some JS object with error details.

filipewl · 2023-09-14T17:14:52Z

src/tracing/errorReporting/ErrorReport.ts

@@ -61,7 +61,7 @@ export class ErrorReport extends ErrorReportBase {
    }

    const serializableError = this.toObject()
-    span.log({ event: 'error', ...indexedLogs, error: serializableError })
+    span.log({ event: 'error', ...indexedLogs, 'error.details': serializableError })


Instead of the JS object mentioned above, we're always getting a boolean for the field error. I think by using a different key, that doesn't conflict with a general "error" name, should fix it.

The span event "error" logs a field called "error" that is expected to contain the error's details, but is instead displaying as a boolean on Honeycomb. This is causing us to miss some important information when debuging an error span. This issue seems to be that a field called "error" is already definded as boolean. A solution would be to rename this field, in this specific span event, to something like "error.details" (we already have "error.id" and "error.kind").

mairatma

@filipewl it might be worth talking to someone from the Observability team to see how to test this. Isn't there a way to force a trace to be created for a request to linked code?

mairatma · 2023-09-19T15:22:01Z

src/tracing/errorReporting/ErrorReport.ts

+      event: 'error',
+      ...indexedLogs,
+      [ErrorReportLogFields.ERROR_MESSAGE]: serializableError.message,
+      [ErrorReportLogFields.ERROR_METADATA]: serializableError.metadata,


Do we need to manually serialize this as well? It returns an object from its type definition.

Good call! I'll flatten this object.

filipewl · 2023-09-19T15:35:57Z

@filipewl it might be worth talking to someone from the Observability team to see how to test this. Isn't there a way to force a trace to be created for a request to linked code?

@mairatma, I figured out this with @arturpimentel. There's a header jaeger-debug-id that you can include in the requests so they won't be sampled and that can be later used on Honeycomb to find your traces. I've updated the PR's description to explain how to test and added a link to a tracing example.

mairatma

Awesome, thanks! Lgtm

Addresses this comment: #543 (comment)

mairatma · 2023-09-19T18:23:30Z

src/tracing/errorReporting/ErrorReport.ts

@@ -65,7 +65,8 @@ export class ErrorReport extends ErrorReportBase {
      event: 'error',
      ...indexedLogs,
      [ErrorReportLogFields.ERROR_MESSAGE]: serializableError.message,
-      [ErrorReportLogFields.ERROR_METADATA]: serializableError.metadata,
+      [ErrorReportLogFields.ERROR_METADATA_METRICS_INSTANTIATION_TIME]: serializableError.metadata.reportCount,


@filipewl is there any chance that metadata may be null or undefined? Just making sure we won't throw here by mistake.

Type-wise, it doesn't look like it can, but I'm afraid of runtime data... I added some optional chaining here to be safe.

Also afraid, thanks!

Type-wise it cannot be null but I'm not sure we can trust with runtime data. Addresses this comment: #543 (comment)

Conflicts: CHANGELOG.md src/tracing/errorReporting/ErrorReport.ts

filipewl added the bug label Sep 14, 2023

filipewl requested a review from Jeymisson September 14, 2023 17:07

filipewl self-assigned this Sep 14, 2023

filipewl commented Sep 14, 2023

View reviewed changes

filipewl force-pushed the fix/tracing-error-log branch from 8dd6a12 to 0edfbe4 Compare September 14, 2023 17:28

filipewl marked this pull request as ready for review September 14, 2023 17:33

filipewl requested a review from arturpimentel September 14, 2023 17:34

Flatten the error object details

0dd7bf3

mairatma reviewed Sep 19, 2023

View reviewed changes

mairatma approved these changes Sep 19, 2023

View reviewed changes

Flatten erro's metadata

eadfe42

Addresses this comment: #543 (comment)

mairatma reviewed Sep 19, 2023

View reviewed changes

filipewl added 2 commits September 19, 2023 15:32

Proctect from possible null/undefined for error.metadata

dc37cf0

Type-wise it cannot be null but I'm not sure we can trust with runtime data. Addresses this comment: #543 (comment)

Merge branch master of github.com into fix/tracing-error-log

5fdcf44

Conflicts: CHANGELOG.md src/tracing/errorReporting/ErrorReport.ts

filipewl merged commit 524bbac into master Sep 20, 2023
2 checks passed

filipewl deleted the fix/tracing-error-log branch September 20, 2023 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error details not being logged in the tracing's span event #543

Fix error details not being logged in the tracing's span event #543

filipewl commented Sep 14, 2023 •

edited

Loading

filipewl Sep 14, 2023 •

edited

Loading

filipewl Sep 14, 2023

mairatma left a comment

mairatma Sep 19, 2023

filipewl Sep 19, 2023

filipewl Sep 19, 2023

filipewl commented Sep 19, 2023 •

edited

Loading

mairatma left a comment

mairatma Sep 19, 2023

filipewl Sep 19, 2023

mairatma Sep 19, 2023

		@@ -61,7 +61,7 @@ export class ErrorReport extends ErrorReportBase {
		}

		const serializableError = this.toObject()

Fix error details not being logged in the tracing's span event #543

Fix error details not being logged in the tracing's span event #543

Conversation

filipewl commented Sep 14, 2023 • edited Loading

What is the purpose of this pull request?

What problem is this solving?

How should this be manually tested?

Types of changes

filipewl Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

filipewl Sep 14, 2023

Choose a reason for hiding this comment

mairatma left a comment

Choose a reason for hiding this comment

mairatma Sep 19, 2023

Choose a reason for hiding this comment

filipewl Sep 19, 2023

Choose a reason for hiding this comment

filipewl Sep 19, 2023

Choose a reason for hiding this comment

filipewl commented Sep 19, 2023 • edited Loading

mairatma left a comment

Choose a reason for hiding this comment

mairatma Sep 19, 2023

Choose a reason for hiding this comment

filipewl Sep 19, 2023

Choose a reason for hiding this comment

mairatma Sep 19, 2023

Choose a reason for hiding this comment

filipewl commented Sep 14, 2023 •

edited

Loading

filipewl Sep 14, 2023 •

edited

Loading

filipewl commented Sep 19, 2023 •

edited

Loading