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

Add Exception.ThreadID field #183

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Add Exception.ThreadID field #183

merged 1 commit into from
Mar 25, 2020

Conversation

Pr0Ger
Copy link
Contributor

@Pr0Ger Pr0Ger commented Mar 14, 2020

Doc says that crashed thread should not have stacktrace, but instead, the thread_id attribute should be set on the exception https://docs.sentry.io/development/sdk-dev/event-payloads/exception/

@rhcarvalho
Copy link
Contributor

Hi @Pr0Ger! There is no "Goroutine ID" available to user code in Go. The docs describe a general protocol we have to adapt to the reality of different languages/platforms.

(In fact, we do have a field Thread.ID that is never set..., and never sent)

@Pr0Ger
Copy link
Contributor Author

Pr0Ger commented Mar 17, 2020

Actually there is, although there is no friendly API for it.

The reason why I thought to add this field is to have stacktraces sent in a consistent way when there is an exception and when it's not

(Yeah, I saw some fields which are not only not used but even is not documented like Exception.RawStacktrace)

@rhcarvalho
Copy link
Contributor

Actually there is, although there is no friendly API for it.

Note that this particular file is part of the HTTP2 implementation and starts with the comment:

https://github.com/golang/net/blob/244492dfa37ae2ce87222fd06250a03160745faa/http2/gotrack.go#L5-L6

// Defensive debug-only utility to track that functions run on the
// goroutine that they're supposed to.

AFAIK Goroutine IDs are never exported, and that's by the design of the language as explained in the FAQ: https://golang.org/doc/faq#no_goroutine_id

The reason why I thought to add this field is to have stacktraces sent in a consistent way when there is an exception and when it's not

Could you elaborate? Do you plan on adding more code to this PR?

@Pr0Ger
Copy link
Contributor Author

Pr0Ger commented Mar 21, 2020

Sure. I'm writing a library to integrate zap logger with sentry. Debug messages are stored as breadcrumbs, messages with error level and higher are sent as events, and if a message has an error field it will be added as an exception to the event.

So, there are the following options:
error message: create an event with stacktrace in in threads
panic/fatal message: add crashed: true to thread
and if the message has an attached error field it will be added to exception

Here is a thing: if I send both thread (without stacktrace) and exception (with stacktrace) it will not be shown in UI (stacktrace section will contain info about thread crashed or not and "No or unknown stacktrace"/"Thread Crashed" instead of stacktrace). Sending stacktrace in both thread and exception works but seems unnecessary. But if I send exception with thread_id UI properly show thread with stacktrace, exception info and crashed flag. Almost the same can be achieved by sending only exception (without thread section) although there will no place to pass crashed flag

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pr0Ger thanks for your note.

Will get this in the next release which is landing end of this week or early next week.

@rhcarvalho rhcarvalho merged commit 406f4d6 into getsentry:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants