Skip to content

Renamed table-manager metrics to remove cortex_dynamo prefix #2307

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

Merged

Conversation

pracucci
Copy link
Contributor

What this PR does:
I've noticed that a couple of table-manager metrics have the cortex_dynamo prefix. I'm proposing to rename them to the cortex_table_manager prefix for clarity.

Which issue(s) this PR fixes:
N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pracucci
Copy link
Contributor Author

@bboreham What's your take on this renaming?

@pracucci pracucci force-pushed the fix-table-manager-metric-names branch from 26aa87c to 3374448 Compare March 20, 2020 17:02
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to rename them more neutrally; I had a couple of comments on wording.
Maybe I'm out of the loop re prefixing metrics with component name, but table_manager doesn't add much, and units is opaque.

Do you want to do cortex_dynamo_consumed_capacity_total at the same time?

@pracucci pracucci force-pushed the fix-table-manager-metric-names branch from fc35d7c to 90cf30d Compare March 23, 2020 06:50
@pracucci
Copy link
Contributor Author

Maybe I'm out of the loop re prefixing metrics with component name, but table_manager doesn't add much, and units is opaque.

I 100% agree on removing the _units since it's opaque (done). The reason I added the prefix cortex_table_manager (even if a bit redundant for the capacity units specific metric) is because if we have a single prefix per service it comes a bit easier to identify metrics exposed by a single Cortex service. Maybe it's just my paranoia, and I'm open to reevaluate it but considering it shouldn't do any harm (not violating any best practice) we may consider to keep it.

@pracucci
Copy link
Contributor Author

Do you want to do cortex_dynamo_consumed_capacity_total at the same time?

I'm not sure it's correct to remove the dynamo wording from it, because it looks to be DynamoDB specific. Am I missing anything? Maybe you were referring to something different?

@bboreham
Copy link
Contributor

I cannot see that the consumed capacity is any more or less DynamoDB-specific than the provisioned capacity.

single prefix per service [...] shouldn't do any harm

Until we move the place a metric is generated, split one service into two, etc.

@pracucci
Copy link
Contributor Author

I cannot see that the consumed capacity is any more or less DynamoDB-specific than the provisioned capacity.

To my understanding, the consumed capacity metrics are increased only when you the DynamoDB client, while the provisioned capacity metric value is set regardless of backend index store used.

single prefix per service [...] shouldn't do any harm
Until we move the place a metric is generated, split one service into two, etc.

Given moving a piece of logic from a service to another doesn't look a frequent operation, we can always rename them, like we're renaming them in this PR. As of today, we have decided to not guarantee any non breaking changes in metrics even after Cortex 1.0, so until this policy won't change doesn't look a big deal to me (given it's not my/our intention to abuse of it, but it's option we'll have).

@bboreham
Copy link
Contributor

the provisioned capacity metric value is set regardless of backend index store used

It's set to zero for everything except DynamoDB. This is not a good basis to argue from.

Any store could have a concept of provisioned and consumed capacity. The ones that I know do are DynamoDB and CosmosDB, but Cortex doesn't yet have any code to set and fetch CosmosDB capacity.

@bboreham
Copy link
Contributor

No wait, it won't be zero, it will be the default flag values of 1000 and 300.
Still, it has no meaning for every store except DynamoDB.

@pracucci pracucci force-pushed the fix-table-manager-metric-names branch from 7c9acd4 to 6b5e2c9 Compare March 30, 2020 09:04
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci requested review from bboreham and tomwilkie March 30, 2020 09:06
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"doing synching tables" doesn't sound right to me, but I guess that text is unlikely to be read by many people.

Signed-off-by: Marco Pracucci <[email protected]>

Co-Authored-By: Bryan Boreham <[email protected]>
@pracucci
Copy link
Contributor Author

"doing synching tables" doesn't sound right to me, but I guess that text is unlikely to be read by many people.

@bboreham I think you already gave me this feedback previously and I lost the fix at some point. Sorry for that. Accepted your suggestion to fix it 🙏

@pracucci pracucci merged commit 427f410 into cortexproject:master Mar 30, 2020
@tomwilkie tomwilkie mentioned this pull request Mar 30, 2020
3 tasks
@pracucci pracucci deleted the fix-table-manager-metric-names branch April 20, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants