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 date and time stamp to log files #595

Merged
merged 17 commits into from
Jan 14, 2022
Merged

Add date and time stamp to log files #595

merged 17 commits into from
Jan 14, 2022

Conversation

ajstewart
Copy link
Contributor

@ajstewart ajstewart commented Dec 22, 2021

@marxide here is a fix for the log files having no time stamp. I think you mentioned previously that maybe the log files should be added to the DB to avoid the glob. This is a glob version so I thought I'd put it up in case you want to use this as a base to do that.

  • Adds date and time stamps to all logs.
  • Adds dropdown menus to log cards on run detail page.
  • Adds API call to load log file text.
  • Also includes fix for temp config file being created on existing runs before the run has been confirmed to go ahead (e.g. trying to run an already running run).
  • Adds Q_CLUSTER_MAX_ATTEMPTS as an option in the env option file.

To do:

  • Documentation updates.

Fixes #593.

- Added date stamp to logs.
- Party way through select field to choose log on run detail page.
- Added API for fetching run logs.
- Removed loading of log text in the view.
- Added dropdown menu for all logs.
- Minor changes to log file functions.
- Split off temp config creation to function.
- Removed duplicated code in run-detail.js.
- Added max_attempts to env template.
- Removed sys import in runpipeline.py.
@ajstewart ajstewart added the enhancement New feature or request label Dec 22, 2021
@ajstewart ajstewart self-assigned this Dec 22, 2021
[skip ci]
- Now confirms that a log file exists via a glob check which should have a length of 1.
- This is because the log file name is no longer known in advance (previously was a os.path file check).
- Also added Mac specific .DS_store to gitignore.
@ajstewart ajstewart marked this pull request as ready for review January 12, 2022 16:54
@ajstewart ajstewart requested a review from marxide January 12, 2022 16:54
Copy link
Contributor

@marxide marxide left a comment

Choose a reason for hiding this comment

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

Looks good, but won't this break existing runs? I think we'd need to add in a glob for the original log.txt if none with timestamps are found.

Or run a script to rename the logs for existing runs. It could extract and add the timestamp from the first line of the log to the filename. While not a database operation, it might be suitable to put in a manually created data migration (i.e. using RunPython) so that it's clear it needs to be run. https://docs.djangoproject.com/en/4.0/topics/migrations/#data-migrations

@ajstewart
Copy link
Contributor Author

Looks good, but won't this break existing runs? I think we'd need to add in a glob for the original log.txt if none with timestamps are found.

Or run a script to rename the logs for existing runs. It could extract and add the timestamp from the first line of the log to the filename. While not a database operation, it might be suitable to put in a manually created data migration (i.e. using RunPython) so that it's clear it needs to be run. https://docs.djangoproject.com/en/4.0/topics/migrations/#data-migrations

Ah yes I meant to comment that it wasn't backwards compatible. I don't mind either way, adding log.txt to the run log glob is a simple way to maintain it.

With the migration you mean to just display a note that run logs need to be updated? Or attempt an automated update method?

@ajstewart
Copy link
Contributor Author

@marxide added support to display the old logs in c3017f1.

@ajstewart ajstewart requested a review from marxide January 13, 2022 12:55
marxide
marxide previously approved these changes Jan 13, 2022
@marxide
Copy link
Contributor

marxide commented Jan 13, 2022

With the migration you mean to just display a note that run logs need to be updated? Or attempt an automated update method?

No, I meant that you can have arbitrary Python functions executed during a Django migration. They are meant for manipulating existing records in the database to suit the new schema changes in the migration, but there's nothing stopping us from writing one of these data migrations ourselves that rename log files. Putting it in as a migration just means it will be executed when python manage.py migrate is run, which we do after we deploy an update.

Including the exising log.txt in the glob should be fine.

- Reverted the views to ignore old log files.
@ajstewart ajstewart dismissed marxide’s stale review January 14, 2022 14:37

Old log handling changed.

@ajstewart
Copy link
Contributor Author

@marxide I've added a migration step to convert old logs, I agree in that it's a neater solution for the old ones.

@ajstewart ajstewart requested a review from marxide January 14, 2022 14:39
- As otherwise I realised any copy error would just result in a crash.
@marxide
Copy link
Contributor

marxide commented Jan 14, 2022

Thanks for doing the migration, that will make updating easier!

@ajstewart ajstewart merged commit d66fe87 into dev Jan 14, 2022
@ajstewart ajstewart deleted the iss593-log-time-stamps branch January 14, 2022 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log files are not date stamped
2 participants