cc @ywwg

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (506a9ba) to head (ff43372).
Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##            main   #5755     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        272     272             
  Lines      22798   22773     -25     
=======================================
- Hits       19283   19257     -26     
- Misses      3172    3173      +1     
  Partials     343     343             

cc @open-telemetry/wg-prometheus

    model.NameEscapingScheme = model.NoEscaping

The escaping scheme is designed to be used as a hint when a UTF-8-capable system is delivering metrics to a system that does not or one that does but has UTF-8 turned off. In this case, the specified escaping scheme setting is used rather than using the default non-round-trippable underscore-replacement scheme. Therefore, this option is not intended to be set to NoEscaping when UTF8 is the default. I can see how this is confusing, I might update the documentation to make this more clear.

(part of the confusion is because the same common library is used by metrics producers and consumers, so it's not clear which side of the conversation these settings apply to)

Some more detail: prometheus will put "escaping=allow-utf-8" in the Accept header if it accepts UTF-8, otherwise it may specify a different escaping scheme. If there is no "escaping" term, the producer should use its own default escaping scheme. (If it set "NoEscaping", then it would send UTF-8 and the metrics would be rejected. (That could be an intended behavior, I suppose)

https://github.com/prometheus/prometheus/blob/main/config/config.go#L482
https://github.com/prometheus/common/blob/main/expfmt/expfmt.go#L168-L184

I am not familiar with this exporter, does it do content negotiation with prometheus?

This is just a wrapper around the Prometheus go client library, and users will still use promhttp to expose metrics endpoints, and should get all content negotiation, etc.

Therefore, this option is not intended to be set to NoEscaping when UTF8 is the default

For our tests, If I only set model.NameValidationScheme = model.UTF8Validation, it still used a replacing scheme (e.g. U__rpc_2e_durations_2e_histogram_2e_seconds from rpc.durations.histogram.seconds) during GatherAndCompare, so I assumed users would need to do the same. Do we need to update that function in client_golang? I would have expected the default to be UnderscoreEscaping, rather than ValueEncodingEscaping. Should we tell users to set it to UnderscoreEscaping instead?

For our tests, If I only set model.NameValidationScheme = model.UTF8Validation, it still used a replacing scheme (e.g. U__rpc_2e_durations_2e_histogram_2e_seconds from rpc.durations.histogram.seconds) during GatherAndCompare

It sounds like GatherAndCompare is not respecting the default. Are you able to find where in the call stack the escaping is happening? We may have missed an entry point. I tried setting some breakpoints and was unable to find where the call was happening. I think it would have to be a call to model.EscapeName.

Should we tell users to set it to UnderscoreEscaping instead?

That's a big question tbh! I selected Values escaping because it is roundtrippable, so producers sending UTF-8 metrics can reliably query them back again without risk of collisions. But this may be unexpected behavior for Prometheus users, in which case we could change the built-in default escaping method in common/model.

It sounds like GatherAndCompare is not respecting the default. Are you able to find where in the call stack the escaping is happening? We may have missed an entry point. I tried setting some breakpoints and was unable to find where the call was happening. I think it would have to be a call to model.EscapeName.

I think it happens from here, with expfmt.NewFormat(expfmt.TypeTextPlain): https://github.com/prometheus/client_golang/blob/97aa0493ebfa75f20fe51ebb32f1e5c5315e5fa1/prometheus/testutil/testutil.go#L301

That should return expfmt.FmtText: https://github.com/prometheus/common/blob/b1880d0dabb633dbf29b999c6a046637efb602fe/expfmt/expfmt.go#L49

Then the encoder calls format.ToEscapingScheme on expfmt.FmtText: https://github.com/prometheus/common/blob/b1880d0dabb633dbf29b999c6a046637efb602fe/expfmt/encode.go#L150

expfmt.FmtText doesn't include an escaping clause, so it returns the global model.NameEscapingScheme: https://github.com/prometheus/common/blob/b1880d0dabb633dbf29b999c6a046637efb602fe/expfmt/expfmt.go#L183, which is model.ValueEncodingEscaping by default.

That eventually is used to call model.EscapeMetricFamily inside of the expfmt encoder: https://github.com/prometheus/common/blob/b1880d0dabb633dbf29b999c6a046637efb602fe/model/metric.go#L195-L195

so this could be fixed by having compare build a format that includes the escaping=allow-utf-8 term. There should probably be a func (format Format) WithEscapingScheme(s model.EscapingScheme) Format{} to add an escaping term to a Format so other people can do this.

prometheus/common#688

@ywwg so the plan is to wait for client_golang to update to the latest prom/common release, then add the escaping option when comparing. We will probably need to remove the existing sanitization with a feature gate.

That's a big question tbh! I selected Values escaping because it is roundtrippable, so producers sending UTF-8 metrics can reliably query them back again without risk of collisions. But this may be unexpected behavior for Prometheus users, in which case we could change the built-in default escaping method in common/model.

This sounds like something we should track separately. Should I open an issue in prom/common?

@ywwg so the plan is to wait for client_golang to update to the latest prom/common release, then add the escaping option when comparing. We will probably need to remove the existing sanitization with a feature gate.

yup, I'll poke people to push out another common release

This sounds like something we should track separately. Should I open an issue in prom/common?

yes that sounds like a good idea

I am able to remove overriding model.NameEscapingScheme with model.NoEscaping now. I was also able to switch to using model.EscapeName, rather than our own escaping function.

But I wasn't able to remove the check for model.NameValidationScheme != model.UTF8Validation in the exporter because I still get errors during testing and when running our example. Do we need more updates to client_golang to allow (but escape) names with dots by default?

Right now common.model is still defaulted to Legacy validation, and when 3.0 comes out we will switch this to UTF-8. Until then, it's necessary to set that value manually, either at the top of tests, or in a func init(). In Prometheus, there are a bunch of tests for legacy behavior that I wanted to keep, so even after 3.0 it'll be necessary to override that value to exercise the old behavior.

Feel free to hit me up on CNCF slack, or we can do a video conference to talk it through.

Great. Just making sure I wasn't missing something