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 RfcUtcTime compilation option #152

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Conversation

Ivansete-status
Copy link
Contributor

@Ivansete-status Ivansete-status commented Sep 4, 2024

Adding a simple option to allow logging using UTC TimeZone instead of the local one

This PR is motivated by these comments: status-im/status-desktop#15198 (comment) and status-im/status-desktop#15198 (comment) :)

README.md Outdated
@@ -392,6 +392,13 @@ Possible values are:

https://en.wikipedia.org/wiki/Unix_time

- `UnixTimeReadable`
Copy link
Contributor

Choose a reason for hiding this comment

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

Other concerns about the reasonableness aside, UnixTimeReadable is a bit of a red herring of an option name.

It's not about the "readability" (RfcTime is readable for example), it's about the UTC.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the essence of Unix time is really that it's the seconds (or similar -- milliseconds or nanoseconds) count. If it's not that, it's not any kind of "Unix time" or epoch time.

@tersec
Copy link
Contributor

tersec commented Sep 9, 2024

I do wonder somewhat why the approach described in status-im/status-desktop#15198 (comment) isn't applicable here:

nwaku is not printing in UTC. Instead, it prints from the local time. What is happening is that the fleets' hosts have the local time set to UTC and therefore, nwaku gets that time zone.

Presumably the issue here is that

  • there's a front-end UX which would be affected by running everything for testing purposes under TZ=UTC foo and
  • these aren't mostly being run en masse as fleet nodes with UTC or GMT setups but intrinsically tend to run on individual laptops/desktops with people's actual local time zone?

Also, UnixTime addresses at least the consistent time reference point.

Other options are to add explicitly logged attributes in whatever timezone or formatting one wants. A bit redundant, but depends how scaffolding-y this is all meant to be.

@igor-sirotin
Copy link

... tend to run on individual laptops/desktops with people's actual local time zone?

@tersec yes, this is exactly the reason.

This is needed for status-desktop application. Since privacy is one of our core principles, we don't want to log anything that disclosures users identity. Yet we would like to get bug reports from them, which involves logs sharing.

So changing OS clock is not an option here.

@Ivansete-status
Copy link
Contributor Author

Ivansete-status commented Sep 9, 2024

I think it looks better now. Thanks for the naming suggestions @tersec ! ( we had a private chat )

@Ivansete-status Ivansete-status changed the title add UnixTimeReadable compilation option add RfcUtcTime compilation option Sep 9, 2024
@@ -454,29 +454,35 @@ proc getSecondsPart(timestamp: Time): string =
res[5] = chr(ord('0') + (tmp mod 10))
res

proc getFastDateTimeString(): string =
proc getFastDateTimeString(useUtc = false): string =
Copy link
Member

Choose a reason for hiding this comment

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

why the default here?

README.md Show resolved Hide resolved
# Cache string representation of zone part
let tmp = datetime.format("zzz")
cachedZoneArray = toArray(6, tmp.toOpenArrayByte(0, 5))
if not useUtc:
Copy link
Member

Choose a reason for hiding this comment

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

there's an if for every formatting statement here - maybe accept that these are separate functions..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much indeed for the comments!
For this particular one, I think it is better to keep it in the same proc to avoid possible leftovers if we change that logic in the future.

# Cache string representation of zone part
let tmp = datetime.format("zzz")
cachedZoneArray = toArray(6, tmp.toOpenArrayByte(0, 5))
if not useUtc:
Copy link
Member

@arnetheduck arnetheduck Sep 19, 2024

Choose a reason for hiding this comment

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

even so, most of it can be avoided..

let timezone =
  if useUtc: "Z"
  else:
     ...
     string.fromBytes(cachedZoneArray)

@Ivansete-status Ivansete-status merged commit 1ac2715 into master Sep 19, 2024
12 checks passed
@Ivansete-status Ivansete-status deleted the allow-utc-readable branch September 19, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants