Share cumulative histogram counts variant for Timer #2082
Conversation
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.micrometer.core.instrument.internal; |
There was a problem hiding this comment.
HistogramFlavor is referred to in PrometheusConfig, so probably can't be internal.
There was a problem hiding this comment.
@jkschneider Actually, I wanted to make this be referred in PrometheusConfig, but I couldn't do so due to backward compatibility. I created this for internal use instead. That's why I was hesitating to do this change as mentioned in #2076 (comment).
Alternatively, we could rename it to something like HistogramType or HistogramStyle and move it to io.micrometer.core.instrument or io.micrometer.core.instrument.config for deprecation and ultimate removal of io.micrometer.prometheus.HistogramFlavor and io.micrometer.opentsdb.OpenTSDBFlavor. Let me know what you think.
There was a problem hiding this comment.
OpenTSDBFlavor is kind of reserved for a different purpose, more expansive than just histogram formatting. Since OpenTSDB is so wide-open on format (kind of like Influx), there are other conventions that may eventually become known to us, and for which we would add an option to OpenTSDBFlavor.
HistogramFlavor on the other I think will always apply only to Prometheus. It's because VictoriaMetrics reads a variant of Prometheus' exposition format with a particular expectation around histogram formatting that this option exists.
There was a problem hiding this comment.
@jkschneider Thanks for the explanation! I didn't know the intention. So are you okay with as-is? Or replacing only io.micrometer.prometheus.HistogramFlavor by renaming? Or any other better way you have in mind?
There was a problem hiding this comment.
I'm not sure. Here's what I'm thinking: the commonality between these two timers isn't as much about the cumulative nature of the stats as it is about support for VictoriaMetrics format. And VictoriaMetrics isn't probably going to support some other cumulative monitoring system.
For example, we could be adding a cumulative mode to the Datadog implementation now that it has rate functions (and distributions which conceptually map to Micrometer histograms). So it would seem awkward if Datadog couldn't use the CumulativeHistogramTimer?
Is it better then to keep the code as it is with some duplication to show that this Victoria format is unique to these two registries than to eliminate duplication and try to explain why the construct isn't generalizable to other cumulative registries?
There was a problem hiding this comment.
Lastly, I'm a bit embarrassed about the capitalization miss on HistogramFlavor in 1.4.0. Sorry about that.
There was a problem hiding this comment.
@jkschneider Thanks for the quick feedback! How about PrometheusTimer instead of CumulativeHistogramTimer? If it is still inappropriate, feel free to close this.
There was a problem hiding this comment.
I'm on the fence on this. What do you think @shakuzen? This comes down to a tradeoff of duplication vs. implementation-specific types in core.
I did add FixedBoundaryVictoriaMetricsHistogram to core, judging that its inherent complexity tipped the scales in favor of deduplication.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.micrometer.core.instrument.internal; |
There was a problem hiding this comment.
OpenTSDBFlavor is kind of reserved for a different purpose, more expansive than just histogram formatting. Since OpenTSDB is so wide-open on format (kind of like Influx), there are other conventions that may eventually become known to us, and for which we would add an option to OpenTSDBFlavor.
HistogramFlavor on the other I think will always apply only to Prometheus. It's because VictoriaMetrics reads a variant of Prometheus' exposition format with a particular expectation around histogram formatting that this option exists.
shakuzen
added
the
release notes
|
A couple things have changed since this was last reviewed that might make a different direction more favorable. |
This PR is similar to #2076, but for the
Timerimplementations.