Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors are not serialized by batchLogRecordProcessor #30205

Closed
cuzzlor opened this issue Jun 27, 2024 · 4 comments
Closed

Errors are not serialized by batchLogRecordProcessor #30205

cuzzlor opened this issue Jun 27, 2024 · 4 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor Monitor, Monitor Ingestion, Monitor Query needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@cuzzlor
Copy link

cuzzlor commented Jun 27, 2024

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @azure/[email protected] for the project I'm working on.

My problem is that logRecord attributes that are instanceof Error are not serialized in a way that preserves the message and stack props.

JSON.stringify(new Error('hello'))
produces
{}

JSON.stringify is used without a replacer that can enumerate the message and stack props of an Error, because they are not enumerable (because JS).

https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/monitor/monitor-opentelemetry/src/logs/batchLogRecordProcessor.ts#L35

In the patch below we are importing our own replacer with the very simple structure which simply spreads the message, stack and other props of the error:
https://github.com/MakerXStudio/node-winston/blob/main/src/serialize-error.ts

Here is the diff that solved my problem:

diff --git a/node_modules/@azure/monitor-opentelemetry/dist/index.js b/node_modules/@azure/monitor-opentelemetry/dist/index.js
index 425f6a4..534ff0d 100644
--- a/node_modules/@azure/monitor-opentelemetry/dist/index.js
+++ b/node_modules/@azure/monitor-opentelemetry/dist/index.js
@@ -34,6 +34,7 @@ var coreClient = require('@azure/core-client');
 var instrumentationBunyan = require('@opentelemetry/instrumentation-bunyan');
 var instrumentationWinston = require('@opentelemetry/instrumentation-winston');
 var sdkLogs = require('@opentelemetry/sdk-logs');
+var { serializableErrorReplacer } = require("@makerx/node-winston/serialize-error");
 
 function _interopNamespaceDefault(e) {
     var n = Object.create(null);
@@ -3611,7 +3612,7 @@ class AzureBatchLogRecordProcessor extends sdkLogs.BatchLogRecordProcessor {
         // Ensure nested log attributes are serialized
         for (const [key, value] of Object.entries(logRecord.attributes)) {
             if (typeof value === "object") {
-                logRecord.attributes[key] = JSON.stringify(value);
+                logRecord.attributes[key] = JSON.stringify(value, serializableErrorReplacer);
             }
         }
         super.onEmit(logRecord);

This issue body was partially generated by patch-package.

@github-actions github-actions bot added customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jun 27, 2024
@jeremymeng jeremymeng added the Client This issue points to a problem in the data-plane of the library. label Jun 27, 2024
@github-actions github-actions bot removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 27, 2024
@github-actions github-actions bot added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jun 27, 2024
@jeremymeng jeremymeng added Monitor Monitor, Monitor Ingestion, Monitor Query and removed Monitor - Distro Monitor OpenTelemetry Distro OpenTelemetryInstrumentation labels Jun 27, 2024
@hectorhdzg
Copy link
Member

@cuzzlor thanks for the details, you should be able to take care of the issue without patching the code using a LogProcessor and formatting the error attributes, I'm wondering how often most people will see Errors as part of LogRecords, can you describe the actual way you are using Winston that generated the problem here?

@cuzzlor
Copy link
Author

cuzzlor commented Jul 20, 2024

Hi @hectorhdzg

I noticed our error logs were missing error details when including an error in the log function arguments:

logger.error('this went wrong', {blah, error})

In this case, blah comes out in the logs, error gets dropped.

@hectorhdzg
Copy link
Member

hectorhdzg added a commit that referenced this issue Sep 6, 2024
…30495)

### Packages impacted by this PR
@azure/monitor-opentelemetry-exporter

### Issues associated with this PR
#30205
@hectorhdzg
Copy link
Member

This should be fixed in latest release, thanks

@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Monitor Monitor, Monitor Ingestion, Monitor Query needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

5 participants