Skip to content

Conversation

@jamescrosswell
Copy link
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 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
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
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
Copy link
Collaborator Author

jamescrosswell commented Sep 9, 2024

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
Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

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

3 participants