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.

Sign up for GitHub

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

Jump to bottom

Archive logfile(s) on successful device locking #1870

Merged
merged 5 commits into from
Sep 7, 2023
Merged

Archive logfile(s) on successful device locking #1870

merged 5 commits into from
Sep 7, 2023

Conversation

MichaelHuth
Copy link
Collaborator

@MichaelHuth MichaelHuth commented Sep 1, 2023 •
edited
Loading

  • if there are large log files the nwb export takes long as the whole logfile is always loaded. Archiving the logfile shortens the logfile length to logdata from the same day.

close #1867

@MichaelHuth MichaelHuth self-assigned this Sep 1, 2023
@MichaelHuth MichaelHuth force-pushed the bugfix/1870-mh_long_new_export_due_to_logfile branch from 977e6f4 to ed3ea28 Compare September 1, 2023 17:24
@MichaelHuth MichaelHuth force-pushed the bugfix/1870-mh_long_new_export_due_to_logfile branch from ed3ea28 to 2fbc1b9 Compare September 5, 2023 15:41
@MichaelHuth MichaelHuth changed the title WIP: NWB: Archive logfile(s) on NWB export Archive logfile(s) on successful device locking Sep 5, 2023
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Sep 5, 2023
@t-b
Copy link
Collaborator

t-b commented Sep 5, 2023

Code looks good. Please have a look at CI and the log file I posted in the issue.

@t-b t-b assigned MichaelHuth and unassigned t-b Sep 5, 2023
t-b
t-b previously approved these changes Sep 5, 2023
View reviewed changes
@MichaelHuth MichaelHuth force-pushed the bugfix/1870-mh_long_new_export_due_to_logfile branch from 2fbc1b9 to 663529e Compare September 5, 2023 17:23
@MichaelHuth MichaelHuth assigned t-b and MichaelHuth and unassigned MichaelHuth and t-b Sep 6, 2023
@MichaelHuth MichaelHuth force-pushed the bugfix/1870-mh_long_new_export_due_to_logfile branch from 663529e to d07a9a4 Compare September 6, 2023 12:13
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Sep 6, 2023
@t-b
Copy link
Collaborator

t-b commented Sep 6, 2023

Works as advertised!

Review:

e33fa66 (Util: Use wave getter function for retrieving log file names, 2023-09-05)

Good.

50eef3f (Util: Factor out splitting of ISO9660 timestamp into components, 2023-09-05)

Nice!

d07a9a4 (DAP: Archive log files on successfull locking of a device, 2023-09-05)

  • The commit message does not mention why we are doing this.
  • ARCHIVE_ONCE should be defined at CalledOnceNames. And maybe AlreadyCalledOnce should have
/// 
/// @param name identififer, see also @ref CalledOnceNames

in the documentation as that is not obvious.

Can we move DAP_ArchiveLogFilesOnLocking to MIES_MiesUtilities.ipf and maybe
rename? Keeping ArchiveLogFile static sounds like a good idea.

@t-b t-b assigned MichaelHuth and unassigned t-b Sep 6, 2023
@t-b t-b self-requested a review September 6, 2023 18:02
t-b
t-b requested changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@t-b t-b left a comment

Choose a reason for hiding this comment

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

See comment.

@MichaelHuth
Util: Raise MAX_LOG_LINESIZE from 64K to 2MB size
d39d028 
- MAX_LOG_LINESIZE is used to determine a buffer for archived log files to
  stay below the target size if a line is added. (soft limit)

Based on a customer provided ZMQ log file it was found that the current size
of 64K is too low as we saw single lines with 1+ MB size. Thus, the constant
is increased.
@MichaelHuth
Util: Add desc to AlreadyCalledOnce to hint constant block
4b59ebd 
- names for calling AlreadyCalledOnce should be defined in
  CalledOnceNames
@MichaelHuth
Archive log files on successful locking of a device
e97ea37 
In
3692b98 (Add archiving of uploaded parts of the Log files, 2023-05-26)
archiving of log files was introduced as part of a periodical upload of the
logs, if the user had enabled that feature.
However, if the user did not enabled that feature no archiving was done.
Still the log file data was appended to exported NWB files. With large log
files this resulted in low performance as on each NWB export the log file
was reloaded and appended.
Thus, we had to move the archiving of log files to a more generic spot and
decouple it from the upload log file dependency.

Log files are archived now when a user successfully locks a hardware device
in the DAEPhys panel. Archiving is only attempted once per MIES run.

- log files are archived if they are bigger than 50 MB
- only the part older than ~1 month is archived
- removed archiving from UploadLogFiles

UploadLogFiles uploads less logfile time than the archived time threshold,
thus there is always sufficient log data left for uploading
@MichaelHuth MichaelHuth force-pushed the bugfix/1870-mh_long_new_export_due_to_logfile branch from d07a9a4 to e97ea37 Compare September 7, 2023 11:53
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Sep 7, 2023
@t-b t-b added the PR:NeedsBackport (Pull requests only) Mark it as requiring a backport to the latest release branch label Sep 7, 2023
@t-b
Copy link
Collaborator

t-b commented Sep 7, 2023

Very nice! And the revised commit message is also a well done writeup.

@t-b t-b self-requested a review September 7, 2023 16:23
@t-b t-b enabled auto-merge September 7, 2023 16:23
@t-b t-b merged commit edb9ce2 into main Sep 7, 2023
16 checks passed
@t-b t-b deleted the bugfix/1870-mh_long_new_export_due_to_logfile branch September 7, 2023 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-b t-b t-b approved these changes

@timjarsky timjarsky Awaiting requested review from timjarsky timjarsky is a code owner

Assignees

@t-b t-b

Labels
PR:NeedsBackport (Pull requests only) Mark it as requiring a backport to the latest release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Initial NWB export takes forever when having a large ZeroMQ logfile
2 participants
@MichaelHuth @t-b