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

.net definition for exception.stacktrace should be chosen and defined #1569

Closed
cg110 opened this issue Nov 17, 2020 · 7 comments · Fixed by #5143
Closed

.net definition for exception.stacktrace should be chosen and defined #1569

cg110 opened this issue Nov 17, 2020 · 7 comments · Fixed by #5143
Labels
enhancement New feature or request

Comments

@cg110
Copy link

cg110 commented Nov 17, 2020

Feature Request

Is your feature request related to a problem?

I was looking over the exception handling for otel.dotnet here:
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Api/Trace/ActivityExtensions.cs#L72

I was wondering why the stack trace is ex.ToString() rather than ex.StackTrace?

I see here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md#stacktrace-representation
there's a suggestion this is something that google cloud does, but it's up-to-the SIG to define. Other listed implementation suggest it's just a stack.

However, ex.ToString() will duplicate lots of extra information that's already in the exception.type and exception.message, and it's possible it doesn't even have the stack trace if the ToString is overridden in some way.

Checking reference source, suggests that ex.ToString() will by default capture the classname (which is probably a close match to
exception.type) the message and the stack trace.

The main difference I can see is that ToString() also goes through any InnerException, I'd have expected that inner exception should be recorded as another exception, (so it may end up recursive)

Or is it handling the scenario where a custom exception has also overridden ToString() to provide more information?

From my own perspective we tend to encourage people to log ex.ToString(), but I've heard of cases where the ex.ToString() can be very long. So I'm not sure it's the right choice for an activity/trace.

Describe the solution you'd like:

Clarity on if stacktrace should be just the stack (as I'd expect) or the full ex.tostring()

Describe alternatives you've considered.

I could create a helper routine that achieves something similar, but I'd expect confusion with having a helper and an opentelemetry api doing similar, but different things.

@cg110 cg110 added the enhancement New feature or request label Nov 17, 2020
@cg110
Copy link
Author

cg110 commented Nov 19, 2020

I think I've found the origin of the problem:
https://cloud.google.com/error-reporting/reference/rest/v1beta1/projects.events/report

It has a message field listed, which is where the list of supported stacks is from. I suspect that exception.ToString() is actually listed as it provides the complete requirement for message:

the message must contain a header (typically consisting of the exception type name and an error message) and an exception stack trace in one of the supported programming languages and formats

Which Exception.ToString() would achieve in one call. I think that OpenTelemetry should just be logging the StackTrace, and then recordexception for the innerexception (if there is one).

@Oberon00
Copy link
Member

Oberon00 commented Dec 30, 2020

Clarity on if stacktrace should be just the stack (as I'd expect) or the full ex.tostring()

You are (understandably) expecting the wrong thing: exception.stacktrace is in fact not the stacktrace of the exception. Instead it is the most-detailed natural string representation of the exception. There was a heated debate about that name in the original PR at open-telemetry/opentelemetry-specification#697 (comment)

@cg110
Copy link
Author

cg110 commented Dec 30, 2020

Unless I'm misreading the definition is:

A stacktrace as a string in the natural representation for the language runtime. The representation is to be determined and documented by each language SIG.

Or it is here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/exceptions.md#attributes

That, to me, says it should be the stack trace. The type and message are already captured. I'd expect innerexceptions to also be captured as separate exceptions, rather than nested into the stack trace (from ToString)

What you're suggesting as the most detailed exception details sounds more like opentrace.

@Oberon00
Copy link
Member

Oberon00 commented Dec 30, 2020

The current definition is maybe ambiguous. But the examples, corresponding implementations in other languages, and the linked discussion make clear that indeed you want the fuller representation, not just the stacktrace as in "list of frames".

To view it from another perspective: Who says that a stacktrace is just a list of frames? E.g. in Java printStackTrace prints not only the frames but all inner exceptions, etc.
The Wikipedia article for stack trace (https://en.wikipedia.org/wiki/Stack_trace) also has a "detailed stack trace" including exception information.

So I think it is fair to say, that a stacktrace is more than a list of frames in this definition in the spec.

@cg110
Copy link
Author

cg110 commented Dec 30, 2020

I guess it depends on context. My expectation is that open telemetry was about adding structure to things, using ex.ToString() feels like an information duplication (eg with exception.type and exception.message)

But also a loss of information for inner exceptions, as they'll end up in the Stacktrace and not in their own exception record, and so loosing some of the ability to locate inner exceptions by type or message

I'll have to read the discussion, but my expectation was to be more structured with data.

I assumed the Java printstacktrace is in its format for console logging (but I admit to not being that familiar with other languages)

@Oberon00
Copy link
Member

Regarding that, there were discussions before the linked one and it was agreed that a structured representation would be desirable but would require far more spec-work, and the unstructured "stacktrace" makes most people already happy.

@cijothomas
Copy link
Member

Linking a related discussion from a non-OTel repo:
microsoft/ApplicationInsights-dotnet#2234 (comment)

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

Successfully merging a pull request may close this issue.

3 participants