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

Fix up metric name conversion #13

Merged
merged 4 commits into from
Aug 16, 2023
Merged

Conversation

hoelzro
Copy link
Contributor

@hoelzro hoelzro commented May 8, 2023

Some metrics have substrings from the original metric name omitted from the resulting Prometheus metric - for example, both ClusterStatus.red and ClusterStatus.yellow become aws_es_cluster_status, which results in duplicate metric registration and a panic. This PR includes tests for various CloudWatch metric names, plus a new algorithm for splitting them up into words and recombining them.

It's missing input characters in situations like "ClusterStatus.red", so
let's add some tests including that and improve the algorithm
Be smarter about detecting internal words within CloudWatch metric names
…to better illustrate its purpose (plus, CloudWatch metric names aren't
100% Pascal-case)
@ripta ripta self-requested a review May 9, 2023 23:43
@frioux
Copy link
Contributor

frioux commented May 10, 2023

Are the names for this backwards compatible or will the names change?

@hoelzro
Copy link
Contributor Author

hoelzro commented May 10, 2023

@frioux I initially thought not, but then I tried again and tested this against the names we use at work - the three that changed are:

  • CPUUtilization was utilization, is now cpu_utilization
  • LoginsLast10Minutes was logins_last10_minutes, is now logins_last_10_minutes
  • LoginsLast10Minutes_all_count was logins_last10_minutes_all_count, is now logins_last_10_minutes_all_count

I'd argue that the first is a bug akin to the ClusterStatus.yellow one I set out to fix. Regarding the latter two, I didn't even see them when I listed metrics in CloudWatch (which is how I missed them in my initial testing), but I'm also fairly indifferent on last10 vs last_10.

This allows users to opt-in to the new metric name conversion algorithm,
defaulting to the legacy implementation.
@hoelzro hoelzro merged commit 3f3fa9a into ZipRecruiter:master Aug 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants