microsoft / ApplicationInsights-node.js

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

[Beta] - Nested objects get serialised as "[object Object]" again #1304

Closed cuzzlor closed 3 weeks ago

cuzzlor commented 2 months ago

Hi,

We are using winston logging + application insights beta.

v beta.12 is failing to serialise nested objects image

v beta.11 still works image

Similar to the last time in behaviour: https://github.com/microsoft/ApplicationInsights-node.js/issues/1169

Dependencies which work:

        "@opentelemetry/api": "^1.8.0",
        "@opentelemetry/instrumentation": "^0.50.0",
        "@opentelemetry/instrumentation-express": "^0.37.0",
        "@opentelemetry/instrumentation-graphql": "^0.39.0",
        "@opentelemetry/instrumentation-ioredis": "^0.39.0",
        "@opentelemetry/instrumentation-tedious": "^0.9.0",
        "@opentelemetry/instrumentation-winston": "^0.36.0",
        "@opentelemetry/sdk-trace-node": "^1.23.0",
        "@opentelemetry/semantic-conventions": "^1.23.0",
        "@opentelemetry/winston-transport": "^0.2.0",
        "applicationinsights": "3.0.0-beta.11",

Using the same deps with beta.12 results in nested objects being serialised as [object Object]

Full config used:

const resource = Resource.EMPTY
resource.attributes[SEMRESATTRS_SERVICE_NAME] = '[REDACTED]'
resource.attributes[SEMRESATTRS_SERVICE_NAMESPACE] = '[REDACTED]'

useAzureMonitor({
  resource,
  instrumentationOptions: {
    azureSdk: { enabled: false },
    mongoDb: { enabled: false },
    mySql: { enabled: false },
    postgreSql: { enabled: false },
    redis: { enabled: false },
    redis4: { enabled: false },
    console: { enabled: false },
    bunyan: { enabled: false },

    http: { enabled: true },
    winston: { enabled: true },
  } satisfies ApplicationInsightsInstrumentationOptions as unknown as OpenTelemetryInstrumentationOptions, // https://github.com/microsoft/ApplicationInsights-node.js/issues/1269
})

const tracerProvider = trace.getTracerProvider()

registerInstrumentations({
  tracerProvider,
  instrumentations,
})
JacksonWeber commented 2 months ago

@cuzzlor Thank you for providing the config and reporting this issue. Could you also provide an example of a log producing the output in the portal you noted above? Thank you!

cuzzlor commented 2 months ago

@JacksonWeber An example of producing a log which would result in a field of the trace customDimensions section showing a value of [object Object] would be any that uses an object as a value in the winston logger 2nd log method parameter for log entry arguments, e.g.:

logger.info('This is my info log message', { foo: 'this will display OK', nested: { foo: 'this nested field value will not be serialised' } } )
mderriey commented 2 months ago

I experience the same issue after upgrading to 3.0.0-beta.12, which forces me to stay on beta.11.

For context, we also use winston as a logging framework, so it's the scenario where App Insights uses diagnostic-channel to get access to winston log entries.

JacksonWeber commented 2 months ago

@cuzzlor @mderriey Thank you for your continued usage and testing of the beta! We're moving away from the https://www.npmjs.com/package/diagnostic-channel-publishers solution for collecting Winston logs and moving to the OpenTelemetry instrumentation. While this won't be available in the release of ApplicationInsights 3.0.0, it's undergoing review here https://github.com/Azure/azure-sdk-for-js/pull/29321.

I'll work on addressing the ability to send these kinds of nested logs in the OpenTelemetry transport for Winston https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/winston-transport. I can alert you both when these changes are completed and the issue of serialization is fully solved (for both our Azure Monitor OpenTelemetry, and Application Insights 3.X SDK offerings).

mderriey commented 2 months ago

I feel like log collection was the main reason for us to use the App Insights SDK, and not the Azure Monitor OpenTelemetry Distro.

Once the serialisation issue is fixed, we could use the OpenTelemetry transport for winston and get the logs shipped to App Insights through the OTel logs.

Is there a page that lists what the App Insights SDK offers on top of @azure/monitor-opentelemetry? I don't think either of us used the 2.x SDK, and we're familiar with OpenTelemetry, but maybe there are built-in features we don't realise we depend on...

Thanks in advance!

JacksonWeber commented 2 months ago

@mderriey The official Azure Monitor OpenTelemetry Distro docs are a good resource for the differences as well as the migration guide for those using Application Insights 2.X APIs, but please let me know if you have any further questions regarding comparing feature sets!

Since the Application Insights 3.X SDK is intended to work as a wrapper around the distro, you'd likely be better served by using that product given your familiarity with OpenTelemetry.

JacksonWeber commented 2 months ago

@mderriey @cuzzlor Just wanted to give you both an update that the PR addressing this on the Azure Monitor Distro side has been merged and will be available in the next release https://github.com/Azure/azure-sdk-for-js/pull/29321.

cuzzlor commented 1 month ago

Hi @JacksonWeber,

I tried updating to v3.0.1 and updated all the @otel packages to latest, I still get this issue.

Can you please clarify the roadmap for this bug being fixed? It burns a lot of time deploying updates that need to be tested then rolled back.

Thanks!

This is the current state which still shows the same serialisation bug.

    "@opentelemetry/api": "^1.8.0",
    "@opentelemetry/instrumentation": "^0.51.0",
    "@opentelemetry/instrumentation-express": "^0.38.0",
    "@opentelemetry/instrumentation-graphql": "^0.40.0",
    "@opentelemetry/instrumentation-ioredis": "^0.40.0",
    "@opentelemetry/instrumentation-tedious": "^0.10.1",
    "@opentelemetry/instrumentation-undici": "^0.2.0",
    "@opentelemetry/instrumentation-winston": "^0.37.0",
    "@opentelemetry/sdk-trace-node": "^1.24.0",
    "@opentelemetry/semantic-conventions": "^1.24.0",
    "@opentelemetry/winston-transport": "^0.3.0",
    "applicationinsights": "3.0.1",

BTW - After speaking with @mderriey I agree we will swap to using Azure Monitor OpenTelemetry without applicationinsights, once you confirm object serialization is fixed and released. Thanks

JacksonWeber commented 1 month ago

@cuzzlor the fix for object serialization will be coming with the next release of the Azure Monitor OpenTelemetry package that Application Insights 3.X SDK also relies on.

I expect a release of that package this week. Once that release is out, I'll make a release of this repo with the updated Azure Monitor OpenTelemetry package.

I'm glad to hear the AzMon OpenTelemetry package will work for you both! It should be more versatile if you're not dependent on legacy App Insights API support.

JacksonWeber commented 1 month ago

@cuzzlor @mderriey The updated distro (1.5.0) and exporter (beta.23) are now available in the Azure SDK. A release of Application Insights based on these packages will be available soon.

cuzzlor commented 1 month ago

Thanks Jackson, will use, appreciate your work and comms thank you. 🙏

JacksonWeber commented 3 weeks ago

Closing as fixed, please reopen if you have any follow up questions or issues.