Skip to content
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

Is there any specific reason to not expose now property in the metric options struct? #1354

Open
myarjunar opened this issue Sep 29, 2023 · 2 comments
Labels

Comments

@myarjunar
Copy link

In one of my unit tests, I am comparing the equality of MetricFamily objects, which include a Counter type metric. Prior to the v1.17.0 release, the CreatedTimestamp field did not include a timestamp value at the time of creation. However, with this recent release, the CreatedTimestamp is automatically populated with the current timestamp at the time of metric creation.

I also noticed that the metric options include a now property. Based on the description in the code, it appears that this property is used for testing purposes internally within the package. I would like to suggest making the now property public in the library. This would provide users like me with the ability to mock the now function, enabling us to conduct comprehensive testing externally.

Would love to hear your opinion on this or if there is indeed a specific reason not to do it that way. Thanks in advance!

@ArthurSens
Copy link
Member

Hmmmm, good point... when implementing this though, we were afraid that people would override the now() function with some super weird function that would eventually trigger weird behaviors in Prometheus. It was basically to avoid users to shooting their own foots.

I understand that it makes testing a lot harder now. WDYT @bwplotka @kakkoyun ?

@bwplotka
Copy link
Member

Thanks! Let's find together a good solution, sorry for breaking your tests.

I am comparing the equality of MetricFamily objects, which include a Counter type metric

How you compare those exactly @myarjunar?

I would love to avoid making test-focused fields public in important APIs and add full compatibility guarantee on those. I think there are ways to avoid it:

  1. We could look on your tests. We are adding this field only to protobuf exposition format and protobuf dto. Protobuf is forward & backward compatible so comparing protobuf wouldn't break your test. But users (even client_golang 2w ago or so) was (wrongly in many cases) comparing protobuf string (text) serialized output with expected strings. This is essentially problem explained in encoding: provide canonical output format golang/protobuf#1121 and we updated client_golang to resolve it in Bump client_model #1323 (by comparing proto objects, NOT text format).
  2. If above is unavoidable we could move now from Opts to some global var (sketchy)
  3. ...or add helpers that "sanitize" the strings for test purposes like we have in https://github.com/prometheus/client_golang/blob/main/prometheus/utils_test.go#L43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants