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 fallback end time for spans in a terminated session #884

Merged
merged 1 commit into from
May 24, 2024

Conversation

bidetofevil
Copy link
Collaborator

Goal

If a cached session is killed by the user, the endTime of the session object will not be populated. In that case, use the terminationTime, followed by the lastHeartbeatTime, followed by the start time to ensure that the span always has an end time

Testing

Add to unit test

@bidetofevil bidetofevil requested a review from a team as a code owner May 24, 2024 07:20
Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 80.90%. Comparing base (be623ba) to head (7ca1981).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #884      +/-   ##
==========================================
- Coverage   80.92%   80.90%   -0.02%     
==========================================
  Files         435      435              
  Lines       11616    11613       -3     
  Branches     1764     1764              
==========================================
- Hits         9400     9396       -4     
  Misses       1438     1438              
- Partials      778      779       +1     
Files Coverage Δ
...mbracesdk/comms/delivery/EmbraceDeliveryService.kt 58.46% <50.00%> (+1.10%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM pending one comment on the start time/end time potentially being the same for some sessions

sessionMessage.session.endTime
?: sessionMessage.session.terminationTime
?: sessionMessage.session.lastHeartbeatTime
?: sessionMessage.session.startTime
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it's going to surprise someone 6 months down the line when they ask why a span has the same start + end time. Would it be better to set a null value, or are there constraints that mean that's not possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully this is not actually needed as terminationTime should always be set if forceQuit is set to true. That said, i added lastHeartbeat and start times just in case.

I agree that start time isn't great, but it's better than 0 (which leads to a negative duration on the backend) and null (ended spans should have an endTime). I'll ping the backend folks when I ping them about why endTime for the session is null for a forceQuit about this as well.

@bidetofevil bidetofevil force-pushed the hho/failed-span-endtime branch from abe03b2 to 7ca1981 Compare May 24, 2024 15:18
@bidetofevil bidetofevil merged commit 6e92cf7 into master May 24, 2024
4 checks passed
@bidetofevil bidetofevil deleted the hho/failed-span-endtime branch May 24, 2024 17:08
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