Skip to content

All exceptions are now added as breadcrumbs on future events - #3584

Merged
jamescrosswell merged 14 commits into
mainfrom
exception-breadcrumbs
Sep 11, 2024
Merged

jamescrosswell merged 14 commits into
mainfrom
exception-breadcrumbs

Conversation

@jamescrosswell

Copy link
Copy Markdown
Collaborator

Resolves #3444

This is a bit redundant if tracing is enabled. In that case there is a notification at the top of the event informing you of any other exceptions(s) that have been encountered within the same trace:

image

@jamescrosswell
jamescrosswell marked this pull request as ready for review September 2, 2024 00:51
Comment thread src/Sentry/Internal/Hub.cs
Comment thread src/Sentry/Internal/Hub.cs Outdated
Comment thread src/Sentry/Internal/Hub.cs Outdated
Comment thread src/Sentry/Internal/Hub.cs Outdated
Comment thread src/Sentry/Internal/Hub.cs Outdated
Comment thread src/Sentry/Internal/Hub.cs Outdated
Comment on lines +393 to +411
var exceptionMessage = exception.Message ?? "";
var formatted = evt.Message?.Formatted;

string breadcrumbMessage;
Dictionary<string, string>? data = null;
if (string.IsNullOrWhiteSpace(formatted))
{
breadcrumbMessage = exceptionMessage;
}
else
{
breadcrumbMessage = formatted;
// Exception.Message won't be used as Breadcrumb message
// Avoid losing it by adding as data:
data = new Dictionary<string, string>
{
{"exception_message", exceptionMessage}
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just double-checking: If I've got my log-level set to Warning and the SDK creates an event out of a log warning:

  • The minimum level is reached and the integration captures an event, skipping the breadcrumb creation
  • From the logging integration we get here
  • There was no exception - we bail early
  • Warnings don't get added as breadcrumbs by neither the logging integration nor the core SDK?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch... have pushed a new commit to fix (the SentryLogger and SentrySink now do create breadcrumbs for events, if no exceptions are associated with those events).

I think that's the correct behaviour since CaptureEvent is also used by CaptureMessage (and I assume we don't want to automatically add breadcrumbs for calls to CaptureMessage):

return hub.CaptureEvent(sentryEvent, configureScope);

@jamescrosswell

jamescrosswell commented Sep 9, 2024

Copy link
Copy Markdown
Collaborator Author

I noticed this in the NLog options:

/// <summary>
/// Determines whether or not to include event-level data as data in breadcrumbs for future errors.
/// Defaults to <see langword="false" />.
/// </summary>
public bool IncludeEventDataOnBreadcrumbs { get; set; } = false;

Initially I thought that was related to this ticket but actually it's just confusion over the use of the word "Event" which, in this context, means the logging event (not a sentry event). This settings determines whether "Logging Event" data gets added as context for breadcrumbs:

if (IncludeEventDataOnBreadcrumbs)
{
if (shouldIncludeProperties)
{
var contextProps = GetAllProperties(logEvent);
data ??= new Dictionary<string, string>(contextProps.Count);
foreach (var contextProp in contextProps)
{
if (contextProp.Value?.ToString() is { } value)
{
data.Add(contextProp.Key, value);
}
}
}
}

So, back to the original plan.

@jamescrosswell
jamescrosswell marked this pull request as draft September 9, 2024 22:12
@jamescrosswell jamescrosswell self-assigned this Sep 9, 2024
@jamescrosswell
jamescrosswell marked this pull request as ready for review September 10, 2024 23:39

@bitsandfoxes bitsandfoxes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

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.

SentrySdk.CaptureException should add a breadcrumb for future exceptions

2 participants