Idiomatic Java timestamps (#180)

[mcumings]

Proposal for changing the API surface area with respect to passing timestamps
into and out of the API, leveraging the (long, TimeUnit) tuple pattern than
was introduced in and encouraged after JDK 1.5.

Intended as a discussion point for Issue #180.

[coveralls]

Coverage increased (+0.4%) to 74.503% when pulling e4e436b on mcumings:master into a0fc805 on opentracing:master.

[yurishkuro]

+1, but I think we need to keep the existing method and mark them @deprecated

[mcumings]

Totally agree. I was stealing cycles to get this pushed up. I'll take another pass and re-introduce the original signatures alongside the modified ones.

[coveralls]

Coverage decreased (-1.3%) to 72.816% when pulling ed35ff7 on mcumings:master into a0fc805 on opentracing:master.

[coveralls]

Coverage increased (+2.3%) to 76.375% when pulling e60107b on mcumings:master into a0fc805 on opentracing:master.

[tylerbenson]

Why was @Deprecated added here?

[mcumings]

The interface which defines the method that is being overridden declares it as @deprecated. Not annotating this class still results in a compile-time warning and IDE static inspection flagging.

[bhs]

I think this is a positive change... somewhat complicated in that TimeUnit usually seems to be reserved for durations (and "duration since the epoch" honestly seems like bulls**t to me... that's pretty much the definition of an absolute timestamp). Nevertheless, constructing Date objects feels like overkill here, and the precision information in the TimeUnit is genuinely useful (esp when transcoding millisecond- or second-granularity data).

I'd propose we merge this in 72h if there are no objections. cc @pavolloffay @yurishkuro since you've looked already.

[yurishkuro]

+1

I don't share the concerns about "duration" vs. "timestamp". Every programmer understands the notion of a timestamp expressed as a number of some time units since epoch. Rather than insisting on one specific time unit that can only be seen from documentation, we're letting the caller specify it and making the API self-describing. In other words, I don't see why span.finish(timestampMicros) is less or more confusing than span.finish(timestamp, TimeUnit.Micros). Both are the unfortunate consequence of inefficient type system that doesn't allow creating strongly typed, allocation-free value+unit combos.

[bhs]

@pavolloffay I just saw your comment on #180... WDYT? Are there reasonable alternatives that (a) allow the caller to specify units, (b) support microsecond precision, and (c) don't involve excess allocations?

[pavolloffay]

My first concern is that TimeUnit is from java.util.concurrent and it is used to model timeouts in various units. For timeouts/scheduled things it makes sense, however, I haven't seen this class being used for timestamps in JDK or any 3rd party library.

Does it make sense to use TimeUnit for timetamps?

Would anybody use following? And how it would call it? There is no easy way how to get minute/hour/day timetamp in java.

span.finish(val, TimeUnit.DAYS);
span.finish(val, TimeUnit.HOURS);
span.finish(val, TimeUnit.MINUTES);
span.finish(val, TimeUnit.SECONDS);

honestly when I see this API without reading the javadocs I think that val is span duration and not timestamp (my primary language is java, so I judge based on my previous experience).

Small example how to get timestamp in seconds:

• System.currentTimeMillis() / 1000
• TimeUnit.SECONDS.convert(val, TimeUnit.MILLISECONDS)
  almost the same applies for other units (the first will be even more awkward).

The most natural timestamp units in java are milliseconds, therefore all APIs are defigned to work weel with them.

I don't see benefits in this API change. It's confusing and more error prone to pass less precise value or even think about it as a span duration.

@mcumings do you have any specific use-case where this API chage simplify usage?

[tylerbenson]

@pavolloffay I agree that the more natural unit would be milliseconds, but the current API is defined as microseconds, not milliseconds, which I think is very confusing and error prone in the java landscape given that it will always require a conversion. This also means that we can't change the units over to milliseconds without a difficult API change.

Regarding TimeUnit.SECONDS.convert(val, TimeUnit.MILLISECONDS)...
First I prefer calling the specific convert method for the units I want toMillis as I think that is more intuitive than just convert.
Second I generally like to static import TimeUnit.* which gives us something like SECONDS.convert(val, MILLISECONDS).
Third, the origin unit will be passed in as an argument in most cases, so the static import would be less necessary, as it would look something like this:

public void myMethod(long duration, TimeUnit units) {
    long durationInMicros = units.toMicros(duration);
    // do the rest as you would with the previous API.
}

[mcumings]

The use case is documented in the issue and in the tests associated with this PR. i.e., it is the specific case you identified: "the most natural timestamp units in java are milliseconds". But an OpenTracing API cannot assume that all consumers and implementors would be okay with losing precision by only working in milliseconds and thus we need a way to specify the data along with its precision.

We could do as you suggested and add an additional method call for each level of precision that we'd like to support, thereby binding the precision up with the method name. This expands the API surface area and would (IMHO at least) feel quite clunky. e.g., Should we add an additional set of methods throughout the API as well in order to support nanosecond precision when an implementation desires it?

At the base of your argument against the use of time units appears to be the fact that the core JDK does not internally use TimeUnit for anything other than time deltas. I think this is a bit chicken and egg. I'm not aware of any APIs added to the JDK in Java 7 that would required such a specification. NIO would be the closest but filesystems don't support sub-millisecond precision (i.e., even millisecond precision has been a problem for some OSes). Java 8 introduces Instant which we both agree would be a better choice but we cannot leverage due to API compatibility levels.

I'm definitely open to alternate suggestions but I think that TimeUnit is a well understood construct, provides the consumer the ability to specify whatever useful precision they want, and preserves the implementation's ability to use the greatest precision possible.

I think the "duration" versus "timestamp" definition split is overly pedantic but if we want to go that route, System.currentTimeMillis() is documented as a "difference" between two timestamps. A difference between two instants of time is what? A duration. 😄

[pavolloffay]

Regarding TimeUnit.SECONDS.convert(val, TimeUnit.MILLISECONDS)...
First I prefer calling the specific convert method for the units I want toMillis as I think that is more intuitive than just convert....

This is just a syntactical sugar... It conceptually does not change anything from what I said.

We cannot define new finish method with the same name e.g. span.finish(long millis), the only way to do it is to change the name e.g. span.finishMilli() like here.

[tylerbenson]

@pavolloffay are you suggesting that instead of this PR, we change the API to look like this:

span.finish(System.currentTimeMillis() * 1000); //<-- the deprecated method that takes micros
span.finishMillis(System.currentTimeMillis()); // <-- new method that takes millis instead of micros

While that is an improvement, I still think the API change proposed in this PR is better.

[mcumings]

Ported this PR to target v0.31.0 here: #200

[pilhuhn]

I think log(long timestamp, TimeUnit timestampUnit, ...) is problematic in a way as the unit is only a single one. Let me explain.
Before that, I don't think this has to do with the name timestamp vs duration(since start of the span).
The problem I see is that this effectively does not have a way to say "3h2m4s20ms" other than providing this in the smallest possible unit, which could be millis or even nanos (JDKs these days all have a nano-second timer).
If you consider millis here, then you'd need to translate the above int millis and then call log(12335,TimeUnit.MS), where the time unit in reality will always be TimeUnit.MS and thus is redundant.