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

Record Exception.Data when calling RecordException #2439

Open
MatisseHack opened this issue Oct 1, 2021 · 14 comments
Open

Record Exception.Data when calling RecordException #2439

MatisseHack opened this issue Oct 1, 2021 · 14 comments
Labels
enhancement New feature or request

Comments

@MatisseHack
Copy link

Feature Request

Has there been an consideration of recording values from an exception's Data property as tags on the event when calling activity?.RecordException(ex)?

Describe the solution you'd like:

I would like to add key/value data to certain exceptions beyond just a message. It would be useful to record this extra data when the exceptions are caught.

Describe alternatives you've considered.

The extra data can obviously be recorded manually, but it seems natural to include this functionality in the RecordException method.

@MatisseHack MatisseHack added the enhancement New feature or request label Oct 1, 2021
@cijothomas
Copy link
Member

The RecordException will be following the spec here : https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md (its currently experimental, and will be subject to breaking changes). We can revisit this once the spec gets to stable.

@pbiggar
Copy link

pbiggar commented Dec 27, 2021

I think this should be reopened. Data attributes are required to be supported by RecordException according to the spec.

Here is the specification for RecordException: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#record-exception (it is directly linked from the spec in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/exceptions.md)

It says:

If RecordException is provided, the method MUST accept an optional parameter to provide any additional event attributes (this SHOULD be done in the same way as for the AddEvent method).

Given that it is a MUST, it seems that currently RecordException does not follow the spec by not having this parameter.

@alanwest
Copy link
Member

Given that it is a MUST, it seems that currently RecordException does not follow the spec by not having this parameter.

@pbiggar Thanks for raising this. I've opened a separate issue #3369. This issue is slightly different in that it is proposing a change in behavior to the existing RecordExpception method to have it record information from the Exception.Data property.

@cijothomas cijothomas reopened this Sep 9, 2022
@cijothomas
Copy link
Member

Exception.Data not recorded, so keeping this open

@mattjohnsonpint
Copy link

It would be useful if the entire exception object was available somewhere, so that it could be retrieved in a processor. Reducing an exception to its type, message, and stacktrace string makes it impossible to recreate the exception or get at values from other properties. For example ArgumentOutOfRangeException has an ActualValue property. Such values wouldn't be available from the Data dictionary.

Additionally, the stack trace string captured in exception.stacktrace is insufficient to create an actual System.Diagnostics.StackTrace instance, if one wants to do anything more advanced with the frames, their IL offsets, etc. One needs the original exception - as in new StackTrace(ex).

@cijothomas
Copy link
Member

LogRecord do provide access to Exception (https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Logs/LogRecord.cs#L262), but Traces do not. There is an open issue about supporing this in runtime repo, will find it and link from here.

@jamescrosswell
Copy link

There is an open issue about supporing this in runtime repo, will find it and link from here.

@cijothomas I'd be interested in that issue, if you're still able to track it down.

I'm wondering if I could maybe make a PR with @mattjohnsonpint's suggestion above.

@cijothomas
Copy link
Member

dotnet/runtime#53641 (comment)

@jamescrosswell
Copy link

jamescrosswell commented Jul 28, 2023

Thanks @cijothomas. Do you think there's any possibility of working around this in the OpenTelemetry API until that issue in the dotnet runtime gets resolved? That issue has been open for a couple of years now.

There are a couple of potential workarounds that I could think of, but I'm not sure how these sit with the general design philosophy of the OpenTelemetry APIs:

  1. RecordException currently stores a couple of different properties of the Exception in tags. Is there any reason not to put a reference to the Exception itself in a tag at the same time?
  2. When initialising OpenTelemetry, you could give SDK users the option to configure an OnRecordException(Activity activity, Exception ex, in TagList tags) callback that gets invoked each time RecordException is called... that would give folks the opportunity to extend the RecordException method to do whatever they wanted (e.g. storing a reference to the Exception in a Tag, if they wanted)
  3. An Activity.GetExceptions() method that returned any Exceptions recorded for the Activity

Do any of those sound feasible?

@cijothomas
Copy link
Member

@jamescrosswell The RecordException is following the OpenTelemetry specification on how to store Exceptions.
Its best to improve Activity class itself to have first class support for Exception (as opposed to storing in tags), which is what the runtime issue is tracking.

(As a workaround, could you see if you can use ILogger to report exceptions, assuming you control the code which currently calls RecordException?. The LogRecord keeps Exception instance directly, allowing more flexibility at the processor/exporter level.

@jamescrosswell
Copy link

As a workaround, could you see if you can use ILogger to report exceptions, assuming you control the code which currently calls RecordException?

Unfortunately not @cijothomas. I'm working on the Sentry OpenTelemetry Tracing integration. Sentry SDK users could be using all kinds of different solutions for logging.

This issue goes beyond the OpenTelemetry .NET API though right? Even if the Exception was available on the Activity class in the dotnet runtime, that wouldn't solve the problem for other OpenTelemetry APIs (Python, Go etc.).

What would have to happen for the OpenTelemetry specification to change?

@cijothomas
Copy link
Member

What would have to happen for the OpenTelemetry specification to change?

Need to open an issue in the opentelemetry-specification repo describing the issue and the suggested improvement.

@jamescrosswell
Copy link

Thanks @cijothomas 👍🏻

@jamescrosswell
Copy link

I found an existing issue in the opentelemetry-specification repo.

It looks like for Java, something like this was implemented already?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants