search

open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
421 stars 251 forks source link

Best practice for using ILogger with exceptions #1851

Closed kchudnovskiy closed 3 weeks ago

kchudnovskiy commented 3 weeks ago

In OpenTelemetry exceptions are recorded as Events on a Span. We have existing C# code which uses ILogger.LogError(ex, "some message"). Using the App Insights SDK this shows up nicely in the exceptions table there. But after switching to using OTel with a collector and Azure Monitor exporter, we have to make adjustments such as adding AttachLogsToActivityEvent() to get the exception added in the right place. I think the main problem is that this ILogger method is just not compatible with OTel. It's a logger, so it should generate logs, not events on a span. But obviously it was designed before OTel. So in the future as there is more OTel adoption, should I expect that ILogger will be updated to better reflect OTel (for example leaving exceptions to Activity.RecordException), or would there be better support for how the existing method generates OTel? I'm interested because I am not sure if I should encourage everyone to switch to Activity.RecordException now (lots of code). Or to use processors and filters to make existing ILogger exception logs show up in App Insights as close as possible to what devs are used to. Thanks!

cijothomas commented 3 weeks ago

OTel supports reporting exceptions via Span (Activity) as SpanEvents. And also via Logging libraries. However, OpenTelemetry does not make a recommendation to end users about which option they should use - i.e whether to use activity.RecordException or logger.Log(ex).

It mostly boils down to the backend you are using.

AzureMonitor (since you mentioned that) supports both ILogger and ActivityEvent way : https://learn.microsoft.com/en-us/azure/azure-monitor/app/opentelemetry-add-modify?tabs=aspnetcore#add-custom-exceptions

Sharing a subset of general differences between both approach:

  1. Reporting Exceptions via Activity.RecordException will automatically benefit from Sampling. i.e if Activity is not sampled-in, then RecordException avoids storing it.
  2. Reporting Exceptions via Activity.RecordException will cause the Exception to be kept inmemory (as ActivityEvent), until the Activity itself ends. For long running activity, this can be an issue. On the other hand, reporting exception via ILogger will make it immediately available for processors/exporters.
  3. There are limits on # of ActivityEvents. This is not enforced at OTEL SDK, but rather done at OTLPExporter. If you are using a lot of events, there is a chance that the exceptions got dropped.

Closing this issue here, as there is nothing actionable for this repo. It'd be nice if some docs can be written about using Logs vs Activity for reporting Exceptions that covers more than just .NET. Ideally, it should be here: https://opentelemetry.io/docs/
If you plan to send a doc update, I am happy to help review.