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 timestamps to server log files #16731

Merged
merged 1 commit into from Jul 27, 2019

Conversation

@ghost
Copy link

commented Jun 27, 2019

All writers are now using the the ISO 8601 timestamp format defined in the game server settings. No initialization necessary to access that variable, so it seems okay to use for all writer channels. Fixes #16729.

@abcdefg30
Copy link
Member

left a comment

Looks good to me otherwise.

OpenRA.Game/Settings.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Outdated Show resolved Hide resolved
@teinarss
Copy link
Contributor

left a comment

Benchmark class will also need to be updated
https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Game/Support/Benchmark.cs#L50

And line 53.

@ghost

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

Benchmark class will also need to be updated
https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Game/Support/Benchmark.cs#L50

And line 53.

@teinarss If I'm reading this correctly, those two lines pass arguments to the functions I just updated. Wouldn't that result in double timestamps?

@netnazgul

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

If it's just Z it means UTC+0, does it really log UTC values? If it's not, then Z in timestamp is misleading:

If the time is in UTC, add a Z directly after the time without a space. Z is the zone designator for the zero UTC offset. "09:30 UTC" is therefore represented as "09:30Z" or "0930Z". "14:45:15 UTC" would be "14:45:15Z" or "144515Z".
...
The following times all refer to the same moment: "18:30Z", "22:30+04", "1130−0700", and "15:00−03:30". Nautical time zone letters are not used with the exception of Z. To calculate UTC time one has to subtract the offset from the local time, e.g. for "15:00−03:30" do 15:00 − (−03:30) to get 18:30 UTC. 
@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Baking the timestamps in at the lowest log level is a bad idea. This will probably break syncreports (different clients will have different timestamps, so every line will be different) and may break the tree structures in perf.log.

@ghost

This comment has been minimized.

Copy link
Author

commented Jun 29, 2019

@pchote Okay, I don't know what syncreports are, but I see that they have a different channel string than servers do. I've made it so that the timestamp inclusion that I am proposing is conditionally applied only to the "server" channel. All other channels will go unchanged. Also, I've removed the "Z" from the end of the timestamp string otherwise we'd have to use DateTime.UtcNow for that to be valid. The server console logs do not currently use DateTime.UtcNow, but they do use that timestamp string, so rather than changing those logs to use DateTime.UtcNow, I've made the server file logs conform instead. People can still mimic this behavior by changing their system time to UTC+0 and adding the Z to the end of that string in their dedicated server config file.

@ghost

This comment has been minimized.

Copy link
Author

commented Jun 29, 2019

I've updated it to specify whether to add auto-timestamps when initializing the channel. It's rebased and should be good for a merge now.

@ghost

This comment has been minimized.

Copy link
Author

commented Jun 30, 2019

I almost forgot to address @teinarss' request. I went ahead and did as requested, rebased, and pushed. Will that be all?

OpenRA.Game/Support/Benchmark.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Show resolved Hide resolved

@ghost ghost changed the title Add timestamps to log files Add timestamps to server log files Jul 1, 2019

@pchote pchote added this to the Next Release milestone Jul 5, 2019

@teinarss teinarss self-requested a review Jul 10, 2019

@pchote
Copy link
Member

left a comment

Sorry for taking so long to get to this. The overall approach looks fine, but there are a few specific things that should be changed before we merge this:

OpenRA.Game/Support/Log.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Outdated Show resolved Hide resolved
OpenRA.Game/Support/Log.cs Outdated Show resolved Hide resolved
4mfie
Add timestamps to server log files
Servers are now writing timestamps to the log files using the the ISO 8601 timestamp format defined in the game server settings.
@teinarss
Copy link
Contributor

left a comment

LGTM

@pchote
pchote approved these changes Jul 27, 2019

@pchote pchote merged commit ff02b8b into OpenRA:bleed Jul 27, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.