google/flogger

Use java.time.Duration for atMostEvery in LoggingApi.

nstandif opened this issue · 2 comments

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.

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).

(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.