adr-87: customer facing metrics naming policy

[JameelB]

ADR-87: Customer Facing Metrics Naming Policy

This ADR proposes a naming policy that can be applied to all of our customer facing metrics so that they are consistent and clear to our end users.

See related issue: https://github.com/bf2fc6cc711aee1a0c2a/architecture/issues/61

[JameelB]

Thanks for the feedback @tombentley!

@emmanuelbernard also agreed on having an architecture pattern for this so I was thinking of adding those details you mentioned above for the naming policy in that document then linking that in the Proposal section of this adr. wdyt?

[emmanuelbernard]

@JameelB you mean the AP points to a section of the ADR? I think I would prefer the other way around intuitively, no strong arguments. To be honest, I think having a full copy is easier to consume.

[JameelB]

@emmanuelbernard I meant the other way around. This ADR point to the AP. The AP will have details/structure of the naming policy. Basing this on @tombentley's comment on this issue.

[JameelB]

This is ready for initial review. Please take a look when you get a chance @pb82 @tombentley @k-wall @StevenTobin @riccardo-forina

[JameelB]

bump for review @pb82 @tombentley @k-wall @StevenTobin @riccardo-forina

[JameelB]

Thank you for writing this @JameelB, it's a big +1 from me. It's been a bit painful to understand which metrics to use to build the dashboard, and the naming convention we have now doesn't help. Any effort in improving the DX has the green light in my book.

The only bit I'm not sure I like is the rate5m part for rate metrics like |rhosak_incoming_bytes_rate5m. As a user, I'd probably like to ask for the aggregation through a parameter, but I understand the challenge this would force us to address.

Thanks for the review @riccardo-forina! Allowing the aggregation period to be specified via a parameter can certainly be separated from this adr. But what do you think of not including this in the name? Instead of rhosak_incoming_bytes_rate5m, it'll just be rhosak_incoming_bytes_rate. The aggregation period for rate metrics will be documented for now, @tombentley also mentioned this at a previous point.

how important it is to the customer to know the aggregation period? If so, should it be part of the metric name, or is is sufficient to have it in the documentation?

If we don't include it in the name, it would provide us flexibility to allow users to specify the aggregation period at a later stage when we decide to do that without having to change the name of the metric. For other services that wish to use this pattern, they could also consider this feature in their initial design of their own metrics API.

[pb82]

@riccardo-forina @JameelB the rate5m in the name indicates that the metric is based on a recording rule using this timespan. I'm not sure if there is a (good) way to parameterize recording rules. And just calling the metric rhosak_incoming_bytes_rate would raise the question what time span the rate is applied to.

[riccardo-forina]

I think we can keep this for a future ADR if needed, there is nothing inherently wrong in having the rate5m in the name right now.

[JameelB]

@k-wall I've addressed your comments, please have a look again when you get a chance.

@tombentley @StevenTobin @wtrocki Can you take a look at this when you get a chance too? Would like to get your approval here :)

[wtrocki]

LGTM

[JameelB]

@k-wall I've addressed your comments, please take a look again when you get a chance :)

[pb82]

@k-wall are you Ok with approving and merging this ADR? As discussed, implementation details will be in the form of an Epic Brief.

[tombentley]

@machi1990 and @emmanuelbernard are the remaining reviewers who haven't LGTM'd. Any further feedback, or can we now merge this?

[JameelB]

Any updates on getting this merged @tombentley @emmanuelbernard?

[JameelB]

bumping @tombentley @emmanuelbernard :)

[tombentley]

I've LGTM'd. Emmanuel might be able to look at it towards the end of this week and it might be good if he could have another look, but if you're blocked on this then I'm happy to merge it. Let me know!

[JameelB]

I've LGTM'd. Emmanuel might be able to look at it towards the end of this week and it might be good if he could have another look, but if you're blocked on this then I'm happy to merge it. Let me know!

Not blocked on this, can wait for Emmanuel to take a look :) Thanks @tombentley!

[JameelB]

Thanks for the reviews! No other changes on my side. Snce all reviewers have LGTM'ed, please go ahead and merge when you get a chance @tombentley @emmanuelbernard