-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unexport Format strings #576
Conversation
With the addition of the escaping term, format strings now have many permutations and doing direct string comparisons of formats is not reliable. Instead, users should call FormatType and compare the result against the possible enum values. Signed-off-by: Owen Williams <owen.williams@grafana.com>
37e1e12
to
05d7387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it. If this breaks use cases we haven't anticipated, we can still modify or revert.
can you do a new library release with this change? |
Ack, will do it ASAP. |
This change breaks an existing workflow: |
This breaks a lot of things for us... is there an example porting guide? |
We're currently in the process of cutting a new client_golang release. Client_golang 1.19 should work with common 0.48 |
return fmtProtoText | ||
case TypeTextPlain: | ||
return fmtText | ||
case TypeOpenMetrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to generate a fmtOpenMetrics_0_0_1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the new functions in expfmt.go can create these for you: https://github.com/prometheus/common/blob/main/expfmt/expfmt.go#L77
The story is, because the exposition formats are getting more complex, simple string comparison is no longer safe. There are too many equivalent permutations, and therefore things were moved to functions. If the available NewFormat function is not sufficient for your needs, please @ me or file an issue and I can fix it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now TypeOpenMetrics only generates fmtOpenMetrics_1_0_0
If for some reason you need fmtOpenMetrics_0_0_1 there's no way to generate it.
We probably need a TypeOpenMetrics_0_0_1 or an optional version argument for the NewFormat func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see -- yup I can prep a PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like I should also increase the test coverage -- I'll get to this next week. sorry for the breakage in the meantime!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were removed upstream in prometheus/common#576
These were removed upstream in prometheus/common#576
The previous exposed constants are no longer exposed and we can use expfmt.Format instead. prometheus/common#576
* helper/metricsutil: Update usage of expfmt The previous exposed constants are no longer exposed and we can use expfmt.Format instead. prometheus/common#576 * reodered imports --------- Co-authored-by: Matthias Loibl <mail@matthiasloibl.com>
The Now that intermediate libraries are picking this up via client_golang, it is creating headaches for getting dependency trees to converge without replace directives. Can we get this (at least partially) reverted? |
@kylelemons I think you may be overestimating the use, most of the results in your search are inline For example, kubernetes/ingress-gce#2600, seems like they have a very broken depdenabot setup that tries to do everything all at once. |
I can only find one use. |
It's used in many places: https://sourcegraph.com/search?q=context:global++expfmt.FmtText+-file:vendor/github.com/prometheus/client_golang+-file:vendor/k8s.io&patternType=keyword&sm=0 The most common one I've seen is
Maybe this is fixed in main so it's not showing up in sourcegraph? There is no non-alpha release with the fix. |
@beorn7 it sounds like we could restore expfmt.FmtText as a single const for compatibility, and if people experience issues with escaping / utf-8 they will have to update their code anyway |
This is what deprecate is for. https://github.com/search?q=repo%3Agolang%2Fgo+Deprecated%3A+language%3AGo&type=code |
Agreed, this should have been marked deprecated first. |
Revert #576 and add deprecation notice
With the addition of the escaping term, format strings now have many permutations and doing direct string comparisons of formats is not reliable. Instead, users should call FormatType and compare the result against the possible enum values.
I am making this change after breaking a test in prometheus that relied on string matching of formats.