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

Use java.time.Duration for atMostEvery in LoggingApi. #325

Open
nstandif opened this issue Dec 6, 2022 · 2 comments
Open

Use java.time.Duration for atMostEvery in LoggingApi. #325

nstandif opened this issue Dec 6, 2022 · 2 comments
Labels
P3 type=addition A new feature

Comments

@nstandif
Copy link

nstandif commented Dec 6, 2022

Presently, atMostEvery only supports int and java.util.concurrent.TimeUnit. However, the parameters become inconvenient to use for java.time.Duration. Additionally, when using java.time.Duration#toNanos and TimeUnit#NANOSECONDS, the field gets truncated to an int`. This causes the implementer of the duration field to explicitly field depth to avoid overflow, which can become awkward.

I would like atMostEvery to support java.time.Duration, rather than it's present args. This would make more sense on an usability standpoint of this API.

@hagbard
Copy link
Contributor

hagbard commented Dec 6, 2022

Sadly Duration is a recent API and adding support for it would break users on older JDKs.

The reason that it was originally "int" for the value is that it was expected that users would hardcode a value into the log statement, and we wouldn't expect manually chosen values to go out of range (after all "atMostEvery(18000000000000, NANOSECONDS)" is not readable compared to "atMostEvery(5, HOURS)").

What's your use case by which a Duration is needed? Is it a non-constant value for some reason (this is permitted but unusual).

@hagbard
Copy link
Contributor

hagbard commented Dec 6, 2022

(of course by "recent" I mean Java 8, but Flogger predates that and was for a long time 1.6 compatible).

The other reason something like Duration wasn't used is that using a split count/unit approach means that there's no allocation at the call site, which helps when logging is rate limited.

A log statement containing:

atMostEvery(Duration.ofHours(2))

in an inner loop could make potentially millions of Duration instances to pass into the log statement.

And once you decide to cache that value (e.g. in a static field) you get:

atMostEvery(TWO_HOURS)

which is hardly different to:

atMostEvery(2, HOURS)

I'm not saying it'll never happen, but it will mean bumping Flogger to Java 8, and we'll need to understand who that might break.

@kluever kluever added type=addition A new feature P3 labels Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 type=addition A new feature
Projects
None yet
Development

No branches or pull requests

3 participants