Channyboy commented 3 years ago

597

donbourne commented 3 years ago

To sum up discussion yesterday with @Channyboy , there's no way to be 100% consistent in our naming of methods and names used in export format to describe this new sum across histograms / timers / simpleTimers without somewhat needlessly breaking backwards compatibility. So here's what we're suggesting is about as good as we can do for consistency:

                histogram           timer                       simpleTimer
        api
                extends Summable    extends Summable            extends Summable
                long getSum()       long getSum()               long getSum()    
                                    Duration getElapsedTime()   Duration getElapsedTime()

        json field
                sum                 sum                         elapsedTime

        om suffix
                <units>_sum         seconds_sum                 elapsedTime_seconds

One part that will feel inconsistent, IMO, is that, in JSON format, timers have sum and simpleTimers have elapsedTime for the same concept. The alternative would have been to make histograms and timers have different json field names which seemed odd since histograms and timers both have the same set of metrics coming from what OM format calls a summary.

The other part that will feel inconsistent is that, in OM format, timers have a metric with a suffix of seconds_sum (as dictated by OM's summary expo format), whereas simpleTimers have a metric with a suffix of elapsedTime_seconds (which isn't part of a summary) for the same concept. Again, we could have changed simpleTimer's suffix, but that seems like a needless break of backward compatibility.

donbourne commented 3 years ago

We're already breaking the API's backward compatibility in a few places, and API breakage is easy to spot when you compile, so I'm less concerned about one more small break there.

We haven't broken grafana dashboard backward compatibility yet in MP Metrics 3.0, and it feels like doing it just so SimpleTimer and Timer can use the same suffix on sums might be annoying to users.

Would it make sense to break backwards compatibility in the API but keep the export formats unchanged, as in the following?

                histogram           timer                       simpleTimer
        api
                long getSum()       Duration getSum()           Duration getSum()

        json field
                sum                 sum                         elapsedTime

        om suffix
                <units>_sum         seconds_sum                 elapsedTime_seconds

donbourne commented 3 years ago

Looking at the code changes it occurs to me that there isn't really much motivation (now that we've eliminated Summable) to change SimpleTimer and Timer from getElapsedTime to getSum. IMO, avoiding that change doesn't affect understandability of those APIs and it would avoid the breaking change to the SimpleTimer API. So I'll refine my previous suggestion to...

                histogram           timer                       simpleTimer
        api
                long getSum()       Duration elapsedTime()      Duration elapsedTime()

        json field
                sum                 elapsedTime                 elapsedTime

        om suffix
                <units>_sum         seconds_sum                 elapsedTime_seconds

jmartisk commented 3 years ago

Looking at the code changes it occurs to me that there isn't really much motivation (now that we've eliminated Summable) to change SimpleTimer and Timer from getElapsedTime to getSum. IMO, avoiding that change doesn't affect understandability of those APIs and it would avoid the breaking change to the SimpleTimer API. So I'll refine my previous suggestion to...
                histogram           timer                       simpleTimer
        api
                long getSum()       Duration elapsedTime()      Duration elapsedTime()

        json field
                sum                 elapsedTime                 elapsedTime

        om suffix
                <units>_sum         seconds_sum                 elapsedTime_seconds

+1

eclipse / microprofile-metrics

Include sum value for summary type as per OpenMetrics / Prometheus format #621

597