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

Write date to logs #565

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Conversation

H0L0theBard
Copy link
Contributor

@H0L0theBard H0L0theBard commented Oct 4, 2023

Suggestion from #560

This PR adds dates to logs in the same format which the file name uses.

image

Closes #560

@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code feedback wanted Feedback is wanted whether the changes by this PR are desired labels Oct 4, 2023
@Asha097

This comment was marked as off-topic.

@itscynxx
Copy link
Contributor

itscynxx commented Oct 5, 2023

Replaced my northstar.dll with the provided .dll, played a bit, worked as expected and didn't encounter any issues

image

Copy link
Contributor

@Jan200101 Jan200101 left a comment

Choose a reason for hiding this comment

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

Code looks good, date format uses the long variant 👍

@Jan200101 Jan200101 added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Oct 5, 2023
Copy link
Contributor

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Code looks good, testing shows date now appears in log files.

@ASpoonPlaysGames
Copy link
Contributor

This only affects the log files (and not things like the in-game console) right?

@H0L0theBard
Copy link
Contributor Author

This only shows in logs.

@GeckoEidechse
Copy link
Member

One trade-off is that this change will increase log size due to preventing more characters for each line. Personally I think this trade-off is worth it.
It might also break existing log parsers but that's not our problem.

If there's no opposition to this PR I'll merge it within the next few hours or so.

@ASpoonPlaysGames
Copy link
Contributor

One trade-off is that this change will increase log size

Would it be worth restricting this to only dedicated servers? Listen servers and clients shouldn't really be having the game open for multiple days at a time

@H0L0theBard
Copy link
Contributor Author

Users can sometimes send their logs as a message.txt in the Northstar discord so having the date in the log can help in verifying if it is actually a recent log.

That's basically the only idea I have in why we'd have it for clients and listen servers. We also recently reduced the spam of some lines in a couple of PRs so I don't personally see any issues with adding a little bit extra onto each line.

@itscynxx
Copy link
Contributor

itscynxx commented Oct 5, 2023

Users can sometimes send their logs as a message.txt in the Northstar discord so having the date in the log can help in verifying if it is actually a recent log.

as an alternative if this isn't "enough" reason, I can have spectre check message.txt files, check if they're northstar logs, and ask for an actual log upload

imo it makes less sense to do it this way (spectre way), and instead just keep date in client logs, but it's an alternative

@ASpoonPlaysGames
Copy link
Contributor

Users can sometimes send their logs as a message.txt in the Northstar discord so having the date in the log can help in verifying if it is actually a recent log.

Fair enough, good enough for me

@GeckoEidechse
Copy link
Member

Imma count that whole discussion as primarily in favour of having date in logs on both server and client and merge as is ^^

@GeckoEidechse GeckoEidechse merged commit 9d8bedf into R2Northstar:main Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge feedback wanted Feedback is wanted whether the changes by this PR are desired
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Write date to logs
7 participants