Repository navigation
Conversation
cf74dd1 to
7086e04
Compare
Implement an ABNF-based OpenMetrics 2.0.0 text format parser in expfmt using github.com/pandatix/go-abnf, wired into NewDecoder and ResponseFormat for OpenMetricsVersion_2_0_0. Metric types info and stateset return an error with a TODO until client_model is expanded to represent them. Tests are ported from prometheus/prometheus/model/textparse. Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…parser Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…ummary count Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…variants Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…histogram Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…mp parsing Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…etrics20ToMap Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…etrics2ParseErrors Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
7086e04 to
ae480ef
Compare
|
Should be good to go @dashpole @krajorama |
krajorama
left a comment
There was a problem hiding this comment.
I don't have the time to review this in detail. LLM says it's mostly fine:
Findings
-
Overflow lets an inconsistent native histogram through.
In openmetrics_2_0_parse.go:1179-1215, obs := h.GetZeroCount() + negTotal + posTotal (and the += totals before it) can wrap around in uint64. This input decodes without error:
h{count:2,sum:1,schema:0,zero_threshold:0,zero_count:18446744073709551615,positive_spans:[0:1],positive_buckets:[3]}
Here MaxUint64 + 3 wraps to 2, which equals the count. The fix is to check for overflow while adding, or to compare with bits.Add64. -
Count consistency is only checked for integer histograms. Once any value isn't a plain integer (isFloatHistogram), no native-bucket check against the count runs. So count:3,...,positive_buckets:[99.5] is accepted. The classic +Inf == count check does run for float histograms, so the two paths are inconsistent. count:+Inf with +Inf:+Inf is also accepted as a float histogram; that may be fine, but it's worth an explicit decision.
-
Float vs integer histogram is decided by how the numbers are written. parsePlainUint64/parsePlainInt64 classify a histogram as integer unless a value has ., e or E. The encoder writes integral float counts without .0 (SampleCountFloat: 3 becomes count:3). So a float native histogram with whole-number values round-trips as an integer histogram, and proto.Equal fails. It's either a spec question or an encoder fix. Either way TestOpenMetrics2RoundTrip covers no float histogram and no gauge histogram; the integer gauge native case does round-trip correctly in my probe.
-
Some data is dropped without an error.
Exemplars on gauge, unknown and summary samples are discarded, and so is st@ on gauge and unknown samples (openmetrics_2_0_parse.go:603-645).
Counters keep only the first exemplar. That one is documented, because dto.Counter holds a single exemplar.
It's worth asking whether these cases should error or be documented, rather than lose data unnoticed. -
One flag now controls two rules. In openmetrics_parse.go:877, enforceRuneLimit now also decides whether NaN/Inf exemplar values are rejected. Renaming it to something like isOM1, or splitting it in two, would keep the name honest.
-
Same performance limits as the OM1 parser. Parse time grows quadratically with line length. In my test a 12 KB line took about 0.6 s and a 24 KB line about 2.5 s, identical for OM1 and OM2. Nothing caps line length, which matters when parsing untrusted scrape bodies. This was already accepted in #1015 and the NewDecoder doc says it isn't for high-performance use, but a short note in the OM2 doc might help.
Nits
decode.go:115-119: the om2Scheme logic is correct, because ToEscapingScheme() falls back to underscore escaping when no escaping parameter is given. A one-line comment saying so would explain why the OM1 scheme isn't reused.
Summary sum rejects NaN and negative values, while histogram sum accepts both. This matches the encoder, so it's fine if intended.
Interleaved descriptors (# TYPE a, # TYPE b, b 1, a 1) are accepted. OM1 does the same, so this isn't a regression.
Tests: about 20 near-identical single-case functions (TestOpenMetrics2ParseCompositeSummary, ...ClassicHistogram, ...NativeHistogram, and so on) could be one table-driven test. The error cases already use a table (TestOpenMetrics2ParseErrors).
…gram bucket sum Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…pported metric types Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
… parseExemplar Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…caping, and perf semantics Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…ate TestOpenMetrics2Parse Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
Fixes #812
Stacked on #1015 (OpenMetrics 1.0 ABNF parser).
Implement an ABNF-based OpenMetrics 2.0.0 text format parser in
expfmtusinggithub.com/pandatix/go-abnf, wired intoNewDecoderandResponseFormatforFmtOpenMetrics_2_0_0.Supports UTF-8 metric and label names, inline
st@start timestamps, multiple exemplars, and composite values for summaries, classic histograms, native integer/float histograms, and gauge histograms.infoandstatesetmetric types return an error untilprometheus/client_modelis expanded to represent them. Tests are ported fromprometheus/prometheus/model/textparse.