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

Pick up CAF patch for ISO 8601, add new btest #360

Closed
wants to merge 1 commit into from

Conversation

Neverlord
Copy link
Member

Closes #346.

@awelzel in a nutshell, these changes are:

  • make the fractional second part optional and support 1-9 digits (down to nanoseconds)
  • support Z suffix
  • support UTC offset via [+-]MM:SS suffix

When sending a timestamp without UTC offset (or Z suffix), Broker will parse the timestamp according to the local timezone.

@Neverlord Neverlord force-pushed the topic/neverlord/gh-346 branch 3 times, most recently from 2e03367 to 514e95f Compare May 27, 2023 12:59
@Neverlord
Copy link
Member Author

I though fromisoformat would be a safe thing to use in Python by now, but our CI still has several Python versions where that doesn't work as expected or even not at all. 😒

@timwoj
Copy link
Member

timwoj commented May 29, 2023

I'm going to hold off merging this one until after zeek 6.0 is out, just out of interest of keeping the broker release branch in the current state until then. We can backport it later if desired.

@Neverlord Neverlord marked this pull request as draft May 30, 2023 10:25
@Neverlord
Copy link
Member Author

Then let's revert this to draft for now. This also changes how Broker formats timestamps that it sends to clients, so we probably should at least have a discussion about how we document/communicate this change.


def parse_iso_timestamp(timestamp):
# replace 'Z' and adjust offset format from '[+-]MM:SS' to '[+-]MMSS'
# (note: Python versions >= 3.7 don't need this)
Copy link
Member

Choose a reason for hiding this comment

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

Following up on our earlier conversation Dominik: you can safely assume 3.7+. We bumped our minimum requirement a while back, see here.

I'm about to submit a round of CI updates since Broker's setup has fallen behind compared to Zeek master and I believe at least the (EOL'd) Ubuntu 18.04 that's currently still configured ran Python 3.6.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately it's not that easy: zeek/zeek#3239

@timwoj
Copy link
Member

timwoj commented Jul 31, 2023

6.0 is out, if you want to move this back to a regular PR.

@timwoj
Copy link
Member

timwoj commented Sep 25, 2023

@Neverlord Is this ready for review again?

@Neverlord
Copy link
Member Author

I thought a bit about this while working on other tasks. Originally this was meant as a bugfix, but it's actually breaking clients. For example @simeonmiteff Go lib.

So I think the question is a bit how we go about that breaking change. For the new variant-based approach, I've created new headers that wrap our input/output representations. So it would be an option for us to leave the v1 endpoint untouched and instead of changing the format offer a new v2 endpoint with the new date rendering.

We also need to discuss the precision of the timestamps. Internally, we have nanosecond precision. However, I think some clients won't be happy if they receive more than 6 digits for the second fraction. At least in Python it seems a bit of a pain. So we could limit the output format to microsecond resolution an also could discuss whether we always print the full six digits or not.

@ckreibich
Copy link
Member

Sorry for the late follow-up here, Dominik. My takes:

offer a new v2 endpoint with the new date rendering

Yes, I like that — we should start making use of versioning to handle format changes.

We also need to discuss the precision of the timestamps.

For the Python bindings use-cases I've encountered so far, nanosecond precision seems pretty unimportant. I think 6 digits is sufficient.

@Neverlord
Copy link
Member Author

Neverlord commented Nov 11, 2023

Thanks for the feedback! Closing this then. I can probably re-use (at least parts of) the tests, but I'll re-implement the rendering based on the new broker variants later on.

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.

docs/websocket: ISO 8601 discrepancies
3 participants