microsoft / ApplicationInsights-node.js

Microsoft Application Insights SDK for Node.js
MIT License
321 stars 138 forks source link

[Beta] Custom error types get serialized as empty objects #1285

Closed mderriey closed 3 months ago

mderriey commented 4 months ago

Follow-up to #1246

What

We're experiencing this issue where some errors get serialized as empty object by the App Insights SDK. This was initially fixed in #1248.

Setup:

Sample code:

class InvariantError extends Error {}

logger.error('Unhandled error', error: new InvariantError('This does not appear in App Insights'))

Why

The reason it doesn't appear is that new InvariantError('').constructor.name is InvariantError and not Error, which is the current mechanism to detect error values.

Maybe we could mimic the heuristics from the serialize-error package?

JacksonWeber commented 3 months ago

@mderriey Thank you for submitting this issue, but I'm having some trouble reproducing your results. I have a simple app currently that looks like this:

const express = require('express');
let appInsights = require("applicationinsights");
const winston = require('winston');

const logger = winston.createLogger({
  transports: [
      new winston.transports.Console({
          level: 'error',
          format: winston.format.simple(),
          debugStdout: true,
      })
  ]
});

appInsights.setup("<YOUR-CONNECTION-STRING-HERE>")
.setAutoCollectConsole(true, true)
.start();

class InvariantError extends Error {}
const app = express();
const PORT = 8080;

async function setupRoutes() {
  app.get('/', async (req, res) => {
    logger.error(`Unhandled error: ${new InvariantError('This does not appear in App Insights')}`);
    logger.error(`Unhandled error: ${new Error('This does appear in App Insights')}`);
    res.send('Hello World!');
  });
}

setupRoutes().then(() => {
  app.listen(PORT);
  console.log(`Listening on http://localhost:${PORT}`);
});

And I'm seeing both errors' messages parsed and output to ApplicationInsights. Please let me know if this sample produces the same results for you. If not, let me know some more details of the sample application you're testing with and what version of the beta you're using.

mderriey commented 3 months ago

Hey @JacksonWeber, thanks for getting back to me.

I can see two differences between your setup and mine:

  1. Yours doesn't seem to be using the beta version of the App Insights SDK.
  2. We don't log errors the same way, I think with your sample the template literal means that the JS runtime converts the Error into a string, not the App Insights SDK. Can you please try this way?

    const express = require('express');
    let appInsights = require("applicationinsights");
    const winston = require('winston');
    
    const logger = winston.createLogger({
     transports: [
         new winston.transports.Console({
             level: 'error',
             format: winston.format.simple(),
             debugStdout: true,
         })
     ]
    });
    
    appInsights.setup("<YOUR-CONNECTION-STRING-HERE>")
    .setAutoCollectConsole(true, true)
    .start();
    
    class InvariantError extends Error {}
    const app = express();
    const PORT = 8080;
    
    async function setupRoutes() {
     app.get('/', async (req, res) => {
    -   logger.error(`Unhandled error: ${new InvariantError('This does not appear in App Insights')}`);
    -   logger.error(`Unhandled error: ${new Error('This does appear in App Insights')}`);
    +   logger.error('Unhandled error', { error: new InvariantError('This does not appear in App Insights') });
    +   logger.error('Unhandled error', { error: new Error('This does appear in App Insights') }));
       res.send('Hello World!');
     });
    }
    
    setupRoutes().then(() => {
     app.listen(PORT);
     console.log(`Listening on http://localhost:${PORT}`);
    });

Thanks

JacksonWeber commented 3 months ago

@mderriey Apologies, I had my test app in shim-testing configuration by accident there, thanks! However, I'm still able to see the message in the InvariantError in the CustomProperties of ApplicationInsights on version 11 of the Beta package. image I'm using the sample app above just reconfigured to use the beta package properly and enabling the winston instrumentation option and able to see both errors serialized.

Assuming you're using the same logger.error calls to create Winston logs, can I see how you're initializing/configuring Azure Monitor?

mderriey commented 3 months ago

How weird... 🤔

Assuming you're using the same logger.error calls to create Winston logs

We are, yes, except we use the JSON format for the console transport, but that shouldn't have any impact from my understanding given diagnostic-channel-publishers injects its own transport.

can I see how you're initializing/configuring Azure Monitor?

Sure thing.

Click to expand ```typescript import { Resource } from '@opentelemetry/resources' import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions' import { useAzureMonitor } from 'applicationinsights' import { SetTenantAliasAttributeSpanProcessor } from './tracing/set-tenant-alias-attribute-span-processor' import { TruncateDbStatementAttributeSpanProcessor } from './tracing/truncate-db-statement-attribute-span-processor' import type { ProxyTracerProvider } from '@opentelemetry/api' import { trace } from '@opentelemetry/api' import type { NodeTracerProvider } from '@opentelemetry/sdk-trace-node' import { registerInstrumentations } from '@opentelemetry/instrumentation' import type { InstrumentationOptions as ApplicationInsightsInstrumentationOptions } from 'applicationinsights/out/src/types' import type { InstrumentationOptions as OpenTelemetryInstrumentationOptions } from '@azure/monitor-opentelemetry' import { instrumentations } from './tracing/instrumentations' // Default attributes set on the OpenTelemetry traces const resource = Resource.EMPTY resource.attributes[SemanticResourceAttributes.SERVICE_NAME] = '' resource.attributes[SemanticResourceAttributes.SERVICE_NAMESPACE] = '' useAzureMonitor({ resource, instrumentationOptions: { azureSdk: { enabled: false }, http: { enabled: false }, mongoDb: { enabled: false }, mySql: { enabled: false }, postgreSql: { enabled: false }, redis: { enabled: false }, redis4: { enabled: false }, // NOTE: only winston is enabled winston: { enabled: true }, console: { enabled: false }, bunyan: { enabled: false }, } satisfies ApplicationInsightsInstrumentationOptions as unknown as OpenTelemetryInstrumentationOptions, }) const tracerProvider = (trace.getTracerProvider() as ProxyTracerProvider).getDelegate() as NodeTracerProvider // NOTE: a couple of custom processors tracerProvider.addSpanProcessor(new SetTenantAliasAttributeSpanProcessor()) tracerProvider.addSpanProcessor(new TruncateDbStatementAttributeSpanProcessor()) registerInstrumentations({ tracerProvider, // NOTE: these are additional OTel instrumentations like HTTP, Express, Redis, tedious. instrumentations, }) ```

When I initially looked into this, I found the problem came from the fact that the custom classes that inherit Error have their constructor.name property that is different from Error, see https://github.com/microsoft/ApplicationInsights-node.js/blob/b99d62dc6f6b01af4fb90e1e5d3caeb634b57859/src/shim/logsApi.ts#L121-L122

If I run this in my Node.js REPL, that's what I get:

λ node
Welcome to Node.js v20.11.1.
Type ".help" for more information.
> const { InvariantError } = require('./src/util/invariant')
undefined
> new InvariantError().constructor.name
'InvariantError'

I also confirmed that we're using 3.0.0-beta.11:

λ npm ls applicationinsights
<redacted>@1.0.0 <redacted>
`-- applicationinsights@3.0.0-beta.11

I also just tested it again, and the error isn't serialized correctly.

For completeness, here's the TypeScript code:

export function invariant(condition: any, message: string): asserts condition {
  if (condition) return
  throw new InvariantError(message)
}

export class InvariantError extends Error {}

In a request handler, I added the following bogus invariant assertion:

invariant(1 === 2, 'Boo')

The code that logs these errors is:

logger.error('Unhandled error', {
  error: unwrappedError,
})

I put a breakpoint on that line, and evaluated unwrappedError.constructor.name, which gave back InvariantError.

Here's the result in the portal:

image

JacksonWeber commented 3 months ago

I spent some time investigating this and want to note a few things.

I was able to recreate your experience with the error object being logged as an empty object. However, from messing with the definition of the InvariantError I was able to get the error to properly be serialized in the customDimensions.

So if I defined my custom error class like:

export class InvariantError extends Error {
    constructor(message: string) {
        super(message);
        Object.setPrototypeOf(this, InvariantError.prototype);
    }
};

Adjusting the prototype accordingly so that the object properly identifies as an InvariantError, I do get an "error": {} logged in ApplicationInsights. However, I was able to avoid this by setting class properties on the InvariantError, ex:

export class InvariantError extends Error {
    constructor(message: string) {
        super();
        this.name = "InvariantError";
        this.message = message;
        Object.setPrototypeOf(this, InvariantError.prototype);
    }
};

Hopefully this works as a workaround for the current released version (beta 11), however in testing and investigation I discovered that as a result of https://github.com/microsoft/ApplicationInsights-node.js/pull/1272, Winston logs will be sent to logger.emit() exposed by the OpenTelemetry API without first being properly formatted. I have a PR here: https://github.com/microsoft/ApplicationInsights-node.js/pull/1297 to resolve that issue, and this should be fixed for you in the upcoming release. Tested with custom errors, standard errors, and other objects.

mderriey commented 3 months ago

@JacksonWeber thanks a lot for your investigation.

I'm curious to know why "my" InvariantErrors showed up as empty objects without any additional changes, whereas you had to change the prototype of the object...

Anyway, thanks a lot, we'll test the workaround where we assign properties directly on our error classes instead of relying on the base class constructor.