Extend model.Duration with `Miliseconds()` method as well as `model.{Milisecond,Second, ...}` vars.

[bwplotka]

trafficstars

Hi :wave:

I would propose extending model.Duration. It's super painful to work with and prone to errors. Examples of lack of Seconds or model.Milisecond:

• int64(db.opts.RetentionDuration)/int64(time.Millisecond) to convert to miliseconds in Prometheus
• 15 * 24 * model.Duration(time.Hour)
• model.Duration(time.Duration(DefaultBlockDuration) * time.Millisecond), :scream:

I was thinking about adding Miliseconds method (we use miliseconds in Prometheus a lot!) as well as adding all model.Milisecond model.Days model.Hours model.Seconds model.Microsecond model.Nanosecond vars.

What do you think? Would we accept just contribution? @beorn7 @brian-brazil @SuperQ

[beorn7]

I think that might be worthwhile, but I also think we should finally come to terms if we want to keep maintaining the various packages in common or not. IIRC @brian-brazil suggested to essentially deprecate everything here except expfmt and maybe move the latter back into client_golang (from whence it originally came…).

It doesn't really make sense to refine the packages here if we want to deprecate them ASAP. (But do we?)

[bwplotka]

I think I need more context about why common was created.

Does it have something to do with maintaining compatibility for important packages with proper semantic Go versioning? If yes then we might need to wait for consensus on that side. (e.g for our experiment with @Helcaraxan https://github.com/Helcaraxan/modularise-example-core)

If we want to keep this I would be happy to add extra methods here, no compatibility break anyway.

[beorn7]

@brian-brazil will certainly be able to provide more context. To my knowledge, we created the common repo for code that is shared between other Prometheus (Go) repos, but we do not see it as outward facing libraries, so we are happy to break things here without warning and will only take care about usages within the Prometheus org. This should be clearly called out in the README.md, but currently isn't. Also there is the exception of expfmt, which could be seen as the reference implementation for the Prometheus text format (generation and parsing). (And again, that fact is not really documented, either. We recently had a discussion about #33, where different opinions about the source of truth became apparent.) expfmt was extracted here from client_golang because it was also used by the Prometheus server. That's not the case anymore, but it is still used by the Pushgateway.

The arrival of Go modules made things even more complicated, as we now essentially have to tag versions here to make Go modules work, but the various packages in common are essentially independent but now all tagged together.

[brian-brazil]

The proposal seems fine to me, though I'm not sure we need all of those.

This particular code is shared with at least AM so I don't see an issue with it being here.

On Fri 7 Feb 2020, 10:17 Bartlomiej Plotka, [email protected] wrote:

Hi 👋

I would propose extending model.Duration. It's super painful to work with and prone to errors. Examples of lack of Seconds or model.Milisecond:

• int64(db.opts.RetentionDuration)/int64(time.Millisecond) to convert to miliseconds in Prometheus
• 15 * 24 * model.Duration(time.Hour)
• model.Duration(time.Duration(DefaultBlockDuration) * time.Millisecond), 😱

I was thinking about adding Miliseconds method (we use miliseconds in Prometheus a lot!) as well as adding all model.Milisecond model.Days model.Hours model.Seconds model.Microsecond model.Nanosecond vars.

What do you think? Would we accept just contribution? @beorn7 https://github.com/beorn7 @brian-brazil https://github.com/brian-brazil @SuperQ https://github.com/SuperQ

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prometheus/common/issues/222?email_source=notifications&email_token=ABWJG5QWO3QPKG5IAWVLOEDRBURL5A5CNFSM4KRLCO52YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ILYASCQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWJG5QPRMUWV7YTJBAY6TDRBURL5ANCNFSM4KRLCO5Q .