Fix delimited proto not escaped correctly#809
Conversation
thampiotr
force-pushed
the
fix-escape-delim-proto
branch
from
5dc7746 to
d57ec2e
Compare
thampiotr
force-pushed
the
fix-escape-delim-proto
branch
3 times, most recently
from
986af83 to
61b386b
Compare
expfmt/encode_test.go
Outdated
| verifyCorrectlyEscaped := func(t *testing.T, out string, format Format) { | ||
| require.NotContainsf(t, out, `"foo.metric"`, "format incorrectly escaped: %s", format) | ||
| require.Containsf(t, out, `foo_metric`, "format incorrectly escaped: %s", format) | ||
| require.NotContainsf(t, out, `"dotted.label.name"`, "format incorrectly escaped: %s", format) | ||
| require.Containsf(t, out, `dotted_label_name`, "format incorrectly escaped: %s", format) | ||
| require.Containsf(t, out, `my.label.value`, "format incorrectly escaped: %s", format) |
There was a problem hiding this comment.
I would prefer that we test the actual output (parsed proto, etc) instead of grepping the strings, but this is ok for now. Can you add a todo to make the test more robust? We will also want to test the other escaping methods, round tripping, etc.
There was a problem hiding this comment.
I can convert this into a table test and verify the entire output if that works :)
There was a problem hiding this comment.
Updated the tests, PTAL.
thampiotr
force-pushed
the
fix-escape-delim-proto
branch
2 times, most recently
from
35ae92a to
3976c4e
Compare
|
I was getting this weird tab/space problem when I was working on my own PR and could not figure out where it was coming from. I ended up just changing the test to not get tripped up on it. I think there is some bizarre nondeterminism in the prototext output |
Signed-off-by: Piotr <[email protected]>
thampiotr
force-pushed
the
fix-escape-delim-proto
branch
from
3976c4e to
96a9730
Compare
The protobuf text representation is intentionally nondeterministic as per protocolbuffers/protobuf-go@582ab3d, there is some context and workarounds here: golang/protobuf#1082 In case of this one test, we're really interested in escaping, so I can achieve this by looking at strings in the output, which I reverted to now. More broadly the |
3 participants
I have encountered an issue that is blocking me from upgrading Prometheus to 3.x where despite receiving a content type header of
"application/vnd.google.protobuf; proto=io.prometheus.client.MetricFamily; encoding=delimited; escaping=underscores"I get.in metric and label names.I've noticed there is possibly a missing call to
EscapeMetricFamilyfor delimited proto.I also noticed the test
TestEscapedEncodedid not assert that actual escaping is done, so fixed that too.