Skip to content

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Feb 29, 2024

A zero duration currently panics in chrono: chronotope/chrono#1474

I need support for zero duration granularity to always immediately flush after recording a value, this is very useful for (integration)-tests.

Also changes the flush check from > to >= to always flush with a zero duration granularity.

@Dav1dde Dav1dde force-pushed the dav1d/fix/panic-zero-duration branch from 622677e to 9e27bd7 Compare February 29, 2024 12:44
@Dav1dde Dav1dde requested a review from fpacifici February 29, 2024 12:45
@Dav1dde Dav1dde self-assigned this Feb 29, 2024
Copy link
Collaborator

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

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

Could you provide some context on the issues you are getting in the integration tests?
You should always be able to force a flush of the accumulator in your test no matter on the granularity

@Dav1dde
Copy link
Member Author

Dav1dde commented Mar 4, 2024

Could you provide some context on the issues you are getting in the integration tests?
You should always be able to force a flush of the accumulator in your test no matter on the granularity

The aggregation interval is configurable in Relay via config (as seconds), which ends up getting passed to rust-usage-accountant. In an integration test I want to just change Relay config while at the same time not introduce additional config just for the integration test (something a user should never configure). It is very convenient if I can set the interval to 0 and it just does the right thing, 1) no aggregation 2) immediate flush. Overall this means less code and easier parameterizable tests.

@Dav1dde Dav1dde merged commit 1000027 into main Mar 4, 2024
@Dav1dde Dav1dde deleted the dav1d/fix/panic-zero-duration branch April 5, 2024 14:48
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.

2 participants