Only start a new session span if it's appropriate #986
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @bidetofevil and the rest of your teammates on |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #986 +/- ##
==========================================
+ Coverage 78.97% 79.02% +0.04%
==========================================
Files 416 416
Lines 11045 11065 +20
Branches 1764 1766 +2
==========================================
+ Hits 8723 8744 +21
Misses 1539 1539
+ Partials 783 782 -1
|
| @@ -98,7 +98,9 @@ internal class CurrentSessionSpanImpl( | |||
| if (appTerminationCause == null) { | |||
| endingSessionSpan.stop() | |||
| spanRepository.clearCompletedSpans() | |||
| sessionSpan.set(startSessionSpan(openTelemetryClock.now().nanosToMillis())) | |||
| if (startNewSession) { | |||
There was a problem hiding this comment.
This introduces undesirable behavior where calling sessionSpan.get() afterwards means folks could attempt to write to the span after the session has ended. Setting the reference to null if no new session is created should be enough to fix it?
There was a problem hiding this comment.
Good call. Will fix tomorrow!
There was a problem hiding this comment.
This brings up an interesting point about what happens if we do stuff when there is no session span. Looking at the code, we can't start a new span without a session span, and likely everything else that gets written to the session span will fail.
Before OTel, that data is just lost, right?
I'll put up the fix for this but lets figure this out before I merge this - we should have tests to verify this case and ensure we don't crash
There was a problem hiding this comment.
Added tests to verify no-session-span state. This should be good to go.
60bebb8
to
8949e64
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
|
Differentiated between initialization and whether a session span exists, as there doesn't necessarily have to be one now, if we are in the background and background activity isn't on. |
8949e64
to
09865d4
Compare
Goal
Pass down whether or not ending a session in the CurrentSessionSpan should start a new session, and make the check appropriately based on end type as well as whether background sessions are enabled.
Testing
Added new tests for the condition