Decoder: Remove use of global name validation and add validation #808

ywwg · 2025-07-31T14:09:03Z

This change removes the use of the global variable to determine name validation, using the provided format instead.

Also:

adds validity checks to the text parser, which previously did not check for name validity.
Returns error if the caller tries to create a decoder for prototext format, which is not supported.

Signed-off-by: Owen Williams [email protected]

bwplotka

Thanks for jumping into it!

Let's fix proto + escaping, thanks for tackling this, it's important.

However, given we had some issues/bugs recently and generally it's not trivial to develop with common because many stable libs and projects use this, let's perhaps:

Make separate PRs for separate fixes so it's easier to manager and ensure we didn't miss anything.
Be careful with breaking changes, this PR proposes 2 breaking changes I believe we could avoid.
Don't try to avoid introducing logic that was not asked for (unless I don't have context) -- ideally we keep the maintenance surface minimal

Hope that makes sense 🙈 WDYT?

expfmt/decode.go

bwplotka · 2025-08-01T09:07:16Z

expfmt/decode.go

@@ -72,12 +74,17 @@ func ResponseFormat(h http.Header) Format {

 // NewDecoder returns a new decoder based on the given input format.
 // If the input format does not imply otherwise, a text format decoder is returned.
-func NewDecoder(r io.Reader, format Format) Decoder {
+func NewDecoder(r io.Reader, format Format) (Decoder, error) {


This is scary, let's be careful here, ideally, in common, we don't break API and if we do we do this is, we do this in a dedicated PR and with tests 😱

It's a signature and (hidden) logic breaking change on a popular API that will affect thousands indirectly of downstream projects and libraries. Signature because the new error handling (1), and hidden logic (2) breaking change, because for OpenMetrics format we suddenly return error and not attempt text like we did in the past. For majority of the existing users. the previous implementation, while broken, was "predictably" broken and we don't hear input to change it now, do we?

For (2), are we sure that no OpenMetrics attempt of parsing with text parser worked? I'm not, I believe for some basic metrics it could work just fine and users depended on it. Instead we should probably, long term should add automatic OM decoding here (no urgency though - users should use textparse from Prometheus ideally).

For (1), we don't need error handling if we skip (2) change. Another idea would be to create errDecoder and error on Decode which is what OpenMetrics input would cause to text parse in some cases. Still this would cause (2) breaking change, so I would just keep NewDecoder as is and only try to add safer parsers ONLY if there's strong use case.

If we really need safe decoder factory, we could add new NewSafeDecoder(io.Reader, Format) (Decoder, error), but then ensure default (unknown Format) returns error too.

@beorn7 looked at Prometheus, and it does not use the common library to do parsing. I doubt anyone actually uses this decoder for openmetrics because it is broken for unknown/untyped metrics. I would have to look closer but I would bet it also doesn't support other openmetrics idioms like different quoting characters.

I think if we don't change the return signature, we should add a clear warning in the docstring that the decoder does not support openmetrics.

expfmt/decode.go

bwplotka · 2025-08-01T09:08:32Z

expfmt/encode.go

@@ -153,7 +153,7 @@ func NewEncoder(w io.Writer, format Format, options ...EncoderOption) Encoder {
 	case TypeProtoDelim:
 		return encoderCloser{
 			encode: func(v *dto.MetricFamily) error {
-				_, err := protodelim.MarshalTo(w, v)
+				_, err := protodelim.MarshalTo(w, model.EscapeMetricFamily(v, escapingScheme))


That's the main important bug fix here and it deserves as separate PR please 🙏🏽 🙈

bwplotka · 2025-08-01T09:09:46Z

expfmt/text_parse.go

@@ -574,7 +574,14 @@ func (p *TextParser) readingType() stateFn {
 	if p.readTokenUntilNewline(false); p.err != nil {
 		return nil // Unexpected end of input.
 	}
-	metricType, ok := dto.MetricType_value[strings.ToUpper(p.currentToken.String())]
+	typString := strings.ToUpper(p.currentToken.String())
+	// OpenMetrics uses UNKNOWN for untyped metrics instead of UNTYPED, so we add


Hm, this PR was erroring when somebody tries to use OM with text parse, but we still try to handle OM here? 🤔 What am I missing? Also a candidate for a separate PR that enables OM decoding, if we want

This was a bit of a stupid hack to try to return a better error message when someone does try to do this. I do agree it's in conflict with refusing to decode openmetrics

thampiotr · 2025-08-01T13:22:39Z

I am currently blocked on the issue of "delimited protos now follow the escaping scheme" and have opened a smaller PR that addresses only that issue here: #809

ywwg · 2025-08-01T14:39:34Z

@bwplotka thanks for reviewing this! I agree this PR does too many things and for now we should focus on fixing the escaping bug. But I also think we might want to sit down and decide what the common library should be expected to do: if it can generate output, and has a decoder, should it not support reading that output? I believe we will find more bugs because right now the output is barely tested.

ywwg · 2025-08-01T16:52:23Z

Notes from slack discussion:

merge separate escaping fix PR
add textproto support
add comments to Decode that it does not support OpenMetrics totally, also that the fastest/best experience is the prometheus parsers.
do not change the newdecoder function signature

ywwg · 2025-08-05T14:17:42Z

expfmt/text_parse.go

@@ -126,6 +129,7 @@ func (p *TextParser) TextToMetricFamilies(in io.Reader) (map[string]*dto.MetricF

 func (p *TextParser) reset(in io.Reader) {
 	p.metricFamiliesByName = map[string]*dto.MetricFamily{}
+	p.currentLabelPairs = nil


the fact that this member was not reset does not bode well for the correctness of the whole parser

Yes. It also proves it's not heavily used (:

ywwg · 2025-08-05T14:17:55Z

expfmt/text_parse.go

+		p.currentLabelPairs = nil
+		return nil
+	}
+	if !p.scheme.IsValidLabelName(p.currentLabelPair.GetName()) {


these checks were not done at all previously

bwplotka

Some initial comments, thanks for improving this!

expfmt/decode.go

ywwg · 2025-08-19T13:19:02Z

reworded / renamed again, because this PR keeps morphing. I think it still does something useful?

bwplotka

Thanks!!! LGTM, one nit only.

expfmt/decode.go

bwplotka · 2025-08-19T14:45:44Z

expfmt/text_parse.go

@@ -126,6 +129,7 @@ func (p *TextParser) TextToMetricFamilies(in io.Reader) (map[string]*dto.MetricF

 func (p *TextParser) reset(in io.Reader) {
 	p.metricFamiliesByName = map[string]*dto.MetricFamily{}
+	p.currentLabelPairs = nil


Yes. It also proves it's not heavily used (:

This change removes the use of the global variable to determine name validation, using the provided format instead. Also: * adds validity checks to the text parser, which previously did not check for name validity. * Returns error if the caller tries to create a decoder for prototext format, which is not supported. Signed-off-by: Owen Williams <[email protected]>

ywwg marked this pull request as draft July 31, 2025 14:09

ywwg marked this pull request as ready for review July 31, 2025 14:40

ywwg changed the title ~~Fixes for proto encode/decode~~ Fixes for metrics encode/decode Jul 31, 2025

ywwg mentioned this pull request Jul 31, 2025

prometheus exporter reporting metrics with dots in them open-telemetry/opentelemetry-collector-contrib#39700

Open

ywwg force-pushed the owilliams/escaping branch from 19ac84a to 30bb979 Compare July 31, 2025 20:11

bwplotka requested changes Aug 1, 2025

View reviewed changes

ywwg marked this pull request as draft August 1, 2025 16:06

ywwg changed the title ~~Fixes for metrics encode/decode~~ Add support for prototext decoding Aug 1, 2025

ywwg changed the title ~~Add support for prototext decoding~~ Support specified validation scheme in Decoders Aug 4, 2025

ywwg changed the title ~~Support specified validation scheme in Decoders~~ Add prototext support to Decoder, and fixes Aug 5, 2025

ywwg commented Aug 5, 2025

View reviewed changes

ywwg force-pushed the owilliams/escaping branch from e34a757 to d3a8808 Compare August 7, 2025 19:42

ywwg marked this pull request as ready for review August 7, 2025 19:46

ywwg requested a review from bwplotka August 7, 2025 20:03

bwplotka reviewed Aug 13, 2025

View reviewed changes

expfmt/decode.go Outdated Show resolved Hide resolved

expfmt/decode.go Show resolved Hide resolved

expfmt/decode.go Outdated Show resolved Hide resolved

expfmt/decode.go Outdated Show resolved Hide resolved

ywwg changed the title ~~Add prototext support to Decoder, and fixes~~ Add name validation to text Decoder Aug 19, 2025

ywwg requested a review from bwplotka August 19, 2025 13:18

bwplotka approved these changes Aug 19, 2025

View reviewed changes

ywwg mentioned this pull request Aug 19, 2025

remove global name validation #796

Draft

ywwg changed the title ~~Add name validation to text Decoder~~ Remove use of global name validation and validation to text Decoder Aug 19, 2025

ywwg changed the title ~~Remove use of global name validation and validation to text Decoder~~ Remove use of global name validation and add validation to text Decoder Aug 19, 2025

ywwg force-pushed the owilliams/escaping branch from b6fe849 to 6f7f949 Compare August 19, 2025 15:08

ywwg changed the title ~~Remove use of global name validation and add validation to text Decoder~~ Decoder: Remove use of global name validation and add validation Aug 19, 2025

ywwg force-pushed the owilliams/escaping branch from 6f7f949 to a683105 Compare August 19, 2025 17:15

bwplotka merged commit fc488aa into main Aug 20, 2025
9 checks passed

bwplotka deleted the owilliams/escaping branch August 20, 2025 08:35

bwplotka mentioned this pull request Aug 20, 2025

Add sustainable Go parser in prometheus/common prometheus/OpenMetrics#300

Open

aknuds1 mentioned this pull request Aug 25, 2025

expfmt: Add NewTextParser function #816

Merged

Decoder: Remove use of global name validation and add validation #808

Decoder: Remove use of global name validation and add validation #808

Uh oh!

Conversation

ywwg commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bwplotka Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thampiotr commented Aug 1, 2025

Uh oh!

ywwg commented Aug 1, 2025

Uh oh!

ywwg commented Aug 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywwg commented Aug 19, 2025

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ywwg commented Jul 31, 2025 •

edited

Loading

bwplotka Aug 1, 2025 •

edited

Loading