-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix timespan compression in backend and add bandaid compressing to front-end #4196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but let's still have a look at why the backend doesn't add up these entries. Afaics the tracingPauseInSeconds
is configured to be 60s even. Or does the backend only use this to calculate the total time a user worked, but doesn't actually "compress" multiple time spans that are close together into one?
If there is an easy backend fix for this, I would prefer that instead of the frontend compression.
I totally agree! However, from my understanding, the time spans were already added to the database in that way. So, if it's a bug, we can fix, we still need a way to compress these time spans somehow. But maybe the backend should do this.. I'll try to grab @youri-k today and investigate with him :) |
@daniel-wer @youri-k and I fixed the actual back end issue. I left the front-end compression in the PR, though, since migrating the existing time spans might be too much effort for too little gain right now. Maybe @fm3 can gauge whether it's okay to leave the uncompressed spans in the db. In any way, we can do the migration always later. |
Also we tweaked the compression algorithm to compare the end timestamp (= timestamp + duration) of timespan A with the start timestamp with timespan B. Previously, only the start timestamps were compared 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fixes 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a bug in the backend code which effectively disabled merging of neighboring time spans completely (annotation ids did never match due to missing casting).
This PR fixes that back end issue (thanks again, @youri-k, for finding this!) and adds a band-aid compression to the front-end (~ 9000 time entries are compressed to 9).
URL of deployed dev instance (used for testing):
Steps to test:
Issues: