Project

General

Profile

Actions

Bug #21204

closed

logging panel needs to use a stable sort

Added by Peter Amstutz 8 months ago. Updated 7 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Story points:
-
Release relationship:
Auto

Description

The logging panel sorts by timestamp, but this has the effect that lines with the same timestamp get sorted together in a way that messes up the original order.

This needs to

a) only sort by timestamp and not the rest of the line
b) use a stable sort (i.e. one that doesn't modify the original ordering for equivalent lines)

Because we're merging logs that are already in order, we don't need to sort at all, we just need to perform a merge that takes the next earliest line from each source.


Subtasks 1 (0 open1 closed)

Task #21241: Review 21204-stable-log-sortResolvedPeter Amstutz12/07/2023Actions
Actions #1

Updated by Peter Amstutz 8 months ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz 8 months ago

  • Assigned To set to Stephen Smith
Actions #3

Updated by Peter Amstutz 8 months ago

  • Target version changed from Development 2023-11-29 sprint to Development 2024-01-03 sprint
Actions #4

Updated by Stephen Smith 7 months ago

Changes at arvados|dea8c169f9d38e5a7c518514450c0955648d58bd branch 21204-stable-log-sort
Tests developer-run-tests-services-workbench2: #81

  • All agreed upon points are implemented / addressed.
    • Changed sorting to preserve original ordering of lines of the same type
    • Lines with different types but matching timestamps are sorted by type, the result is that lines of matching timestamp will be grouped by type one after another, with lines within a type preserving original order
    • Added more tests for preserving original ordering of sorted lines within a single type while correctly ordering/grouping matching timestamp lines of differing types
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • none
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • done, added test cases to ensure correct sorting brhavior
  • Documentation has been updated.
    • n/a
  • Behaves appropriately at the intended scale (describe intended scale).
    • Minimal performance impact expected, since only the sort comparator function was tweaked and a reduce/concat replaced with a map/reduce to provide the necessary information for the sort comparator
  • Considered backwards and forwards compatibility issues between client and server.
    • n/a
  • Follows our coding standards and GUI style guidelines.
    • n/a
Actions #5

Updated by Stephen Smith 7 months ago

  • Status changed from New to In Progress
Actions #6

Updated by Peter Amstutz 7 months ago

  • Release set to 69
Actions #7

Updated by Peter Amstutz 7 months ago

  • Release changed from 69 to 67
Actions #8

Updated by Stephen Smith 7 months ago

I cherry picked the 2 commits to the arvados-workbench2 repo at arvados-workbench2|b774da617da87999f68863b90542dd41d9ba0bd3 21204-stable-log-sort

Actions #9

Updated by Peter Amstutz 7 months ago

Stephen Smith wrote in #note-4:

Changes at arvados|dea8c169f9d38e5a7c518514450c0955648d58bd branch 21204-stable-log-sort
Tests developer-run-tests-services-workbench2: #81

  • All agreed upon points are implemented / addressed.
    • Changed sorting to preserve original ordering of lines of the same type
    • Lines with different types but matching timestamps are sorted by type, the result is that lines of matching timestamp will be grouped by type one after another, with lines within a type preserving original order
    • Added more tests for preserving original ordering of sorted lines within a single type while correctly ordering/grouping matching timestamp lines of differing types
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • none
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • done, added test cases to ensure correct sorting brhavior
  • Documentation has been updated.
    • n/a
  • Behaves appropriately at the intended scale (describe intended scale).
    • Minimal performance impact expected, since only the sort comparator function was tweaked and a reduce/concat replaced with a map/reduce to provide the necessary information for the sort comparator
  • Considered backwards and forwards compatibility issues between client and server.
    • n/a
  • Follows our coding standards and GUI style guidelines.
    • n/a

So, this works and I'm fine with it, LGTM.

But I suspect if you moved timestampMatch into sortableLineSortFunc you could eliminate the map/reduce step and also avoid parsing lines in the common case where the lines are the same log type.

In fact you probably don't need to parse the timestamp at all, unless you have a line that doesn't have a leading timestamp you can directly compare two lines with leading timestamps, the timestamp text is fixed-width and lexically sortable already.

Actions #10

Updated by Stephen Smith 7 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF