Skip to content

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Feb 24, 2025

Purpose

See #1312

Short description

Added a new line to the top of the debug info file to show the moment when the error occurred:

TIMESTAMP: 2025-02-24T16:54:14.330+01:00[Europe/Madrid]

This is obtained using a new extra called timestamp, and passed to DebugInfoActivity as the action of the notification shown by the error.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Feb 24, 2025
@ArnyminerZ ArnyminerZ self-assigned this Feb 24, 2025
@ArnyminerZ ArnyminerZ linked an issue Feb 24, 2025 that may be closed by this pull request
1 task
@ArnyminerZ ArnyminerZ marked this pull request as ready for review February 24, 2025 15:56
@ArnyminerZ
Copy link
Member Author

@devvv4ever would this be enough? Or what else is needed? Maybe add it to the zip file or something?

@devvv4ever
Copy link
Member

It should be the the timestamp of date/time when the error occured. The time when the debug file is generated is not that important, but would be a nice addition also. Do we have some kind of possibility to log the time when the actual error occured?

@ArnyminerZ
Copy link
Member Author

Instead of the moment at which it was generated, the timestamp now matches the moment at which the error occurred (aka the error notification was shown). If manually generated, no timestamp is added since I think it really doesn't matter

sunkup
sunkup previously approved these changes Feb 25, 2025
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Works fine 👍

Would be cool to have it even if notifications are turned off, but with the current design we don't get the exception either, so I guess this is the best we can do for now. :)

btw: Would also be cool if you could update PR description with the new changes before requesting a review. It makes it easier to understand what this PR is about for the reviewer, since you don't have to read through the discussion and find all the information. It's valuable now and in the future.

@sunkup sunkup requested a review from rfc2822 February 25, 2025 09:13
devvv4ever
devvv4ever previously approved these changes Feb 25, 2025
Copy link
Member

@devvv4ever devvv4ever left a comment

Choose a reason for hiding this comment

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

Great, thanks, that's good for now and will make it easier for users to track the time of the error in most cases :)

@rfc2822 rfc2822 dismissed stale reviews from sunkup and devvv4ever via aa2b7b6 February 25, 2025 18:54
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Good 👍🏻 I added that the time is shown as local time and UTC, because I think server logs are often UTC and then it's easier to compare

@rfc2822 rfc2822 merged commit 6be42d4 into main-ose Feb 25, 2025
8 checks passed
@rfc2822 rfc2822 deleted the 1312-add-time-stamp-to-log-files branch February 25, 2025 19:00
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.

Add time stamp to log files
4 participants