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

metrics: rename backend_response to threescale_backend_response #917

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Oct 1, 2018

To clearly distinguish it from the upstream API as suggested in #745 (comment)

@davidor davidor requested a review from a team as a code owner October 1, 2018 10:44
So it's not confused with the upstream API
@davidor davidor force-pushed the rename-backend-response-metric branch from 0941654 to f4c754b Compare October 1, 2018 10:47
@davidor davidor requested a review from mikz October 1, 2018 13:48
@@ -7,7 +7,7 @@ local _M = {}

local backend_response_metric = prometheus(
'counter',
'backend_response',
'threescale_backend_response',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can they start with a number? threescale should be the last resort how to write3scale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that's why I chose threescale :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Sigh :( I really don't like it. Do we have any alternatives? Like prefixing it with something? If no, then let's merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm we might want to prefix all our metrics with apicast. That would solve the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

That does not sound that bad. I'm fine with both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the apicast prefix. I think it makes sense if we take into account that other apps can export metrics to the same Prometheus instance. Also, it's recommended in the Prometheus guidelines: https://prometheus.io/docs/practices/naming/

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@davidor davidor merged commit 849a489 into master Oct 2, 2018
@davidor davidor deleted the rename-backend-response-metric branch October 2, 2018 08:31
@davidor davidor added this to the 3.4 milestone Oct 4, 2018
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.

2 participants