twitter/zipkin — pull request #281 by IceCreamYou (2013-07-29 23:04:17+00:00)
Title: Add aggregates support to the Anorm module

A preemptive note about the getDependencies time range -- CassandraAggregates does this:

val realStart = startDate.floor(1.day).inMicroseconds
val realEnd = endDate.getOrElse(startDate).floor(1.day).inMicroseconds

Whereas AnormAggregates does this:

val startMs = startDate.inMicroseconds
val endMs = endDate.getOrElse(startDate + 1.day).inMicroseconds

What CassandraAggregates does looks clearly wrong to me, and Franklin confirms that it looks broken. Basically right now it says that if you request a start date but do not pass the optional end date, or if you request a start and end date within the same day, it will search within a single microsecond at the beginning of that day. I would humbly suggest that it is not getDependencies' role to enforce flooring to the day; rather it should return dependencies in the exact time range requested, and the caller should choose whether they want dependencies within a day range or not. In any case, the documentation for floor() indicates that it should not be called for values higher than 1 hour or unexpected results could occur due to time zone differences -- so our current usage of floor has an undefined result.

The places where getDependencies are used confirms that this is broken. Currently it is only called from the CassandraAggregatesSpec test and from the /api/dependencies callback in App.scala. The API endpoint has incorrect documentation (it indicates that the parameters should be in seconds) and nonsensical default behavior (if startTime is undefined it retrieves only dependencies from the first microsecond of the current day). The test passes because it only looks for dependencies in the first microsecond of the specified day.

I haven't changed CassandraAggregates yet because I wasn't sure how the Cassandra lookup works and a brief attempt to change it caused test failures I didn't know how to correct.