Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.03k stars 1.19k forks source link

Errors are not serialized by batchLogRecordProcessor #30205

Open cuzzlor opened 2 months ago

cuzzlor commented 2 months ago

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @azure/monitor-opentelemetry@1.6.0 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.

hectorhdzg commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

Related PR https://github.com/open-telemetry/opentelemetry-js-contrib/pull/2352