Client health #1013
Client health #1013
Conversation
|
It would be great if Capture message with event-level Info and Debug wouldn't count as errors on Sessions. |
We defined in the protocol that any event captured counts as errored |
| /// <summary> | ||
| /// Session auto-generated ID. | ||
| /// </summary> | ||
| string Id { get; } |
There was a problem hiding this comment.
I want to replace this with SentryId for consistency. Waiting on reply on Sentry discord.
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
|
@bruno-garcia Okay I think I've resolved everything. Please skim over/double check. |
| CacheDirectoryPath = tempDirectory.Path, | ||
| Release = "test" | ||
| }); | ||
| using var fixture = new Fixture(); |
There was a problem hiding this comment.
xunit will call dispose on test classes that implement IDisposable so if you prefer you can define the fixture as a field (and implement IDisposable).
| DiagnosticLogger = Logger | ||
| }; | ||
|
|
||
| SessionManager = new GlobalSessionManager(Options); |
There was a problem hiding this comment.
nit: You could have a method that returns GlobalSessionManager (usually called GetSut) so that tests get a hold of the newly created reference instead of going through fixture.Xyz and control when to create it, so they can mutate Options before actually creating the instance.
| { | ||
| if (_currentSession is { } session) | ||
| { | ||
| session.ReportError(); |
There was a problem hiding this comment.
Technically this could be an 'ended' session since another thread could have raced and closed/also called reported error on it. We either need to be able to flip the errors atomically or we need to sync all methods here to have a consistent view of things.
Considering worst case scenario is that we double report a session with the same state (errored+closed or errored+2) this isn't critical so we can merge as-is.
| if (sessionUpdate is not null) | ||
| { | ||
| CaptureSession(sessionUpdate); | ||
| } |
There was a problem hiding this comment.
When can it return null here? Should we log Debug?
There was a problem hiding this comment.
If the release is unset for example. It's already logged inside session manager.
| @@ -189,8 +234,27 @@ public SentryId CaptureEvent(SentryEvent evt, Scope? scope = null) | |||
| evt.Contexts.Trace.ParentSpanId = linkedSpan.ParentSpanId; | |||
| } | |||
|
|
|||
| // Report an error on current session if contains an exception | |||
| var sessionUpdate = evt.Exception is not null | |||
There was a problem hiding this comment.
The edge case here is that if this was a capture event that came as a result of a manually crafted event or even deserializing an already proceeded event, this instance (evt.Exception) will be null but SentryException wouldn't be null so ideally this could be:
| var sessionUpdate = evt.Exception is not null | |
| var sessionUpdate = evt.Exception is not null || evt.SentryException?.Any() == true |
| ? _sessionManager.ReportError() | ||
| : null; | ||
|
|
||
| // Only set the session if the error count changed from 0 to 1. |
Notes:
Increment session error count on all events (with or without exception, so includesCaptureMessage(...)too)StartSession()should close previous session if it hasn't been closedIHubcan be separated fromISentryClientto make it easier to passSessiontoCaptureEvent(...)Dev docs: https://develop.sentry.dev/sdk/sessions/
User docs: https://docs.sentry.io/product/releases/health/
Cocoa reference: https://github.com/getsentry/sentry-cocoa/blob/master/Sources/Sentry/SentrySession.m