-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add internal label for tracking active series #30
Conversation
pkg/influx/parser.go
Outdated
lbls = append(lbls, cortexpb.LabelAdapter{ | ||
Name: labels.MetricName, | ||
Value: name, | ||
}) | ||
lbls = append(lbls, cortexpb.LabelAdapter{ | ||
Name: "_original_format", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name: "_original_format", | |
Name: "__original_source__", |
I'd rename this label to __original_source__
for a few reasons:
- An internal label in Prometheus is actually prefixed by two underscores. The suffix just makes it more regular with the others like
__name__
. - Although we're thinking of this label as "internal" its not actually internal because its not recognized by prometheus or cortex themselves. That said we could actually upstream this change to cortex, since there will be other proxies translating from other sources. So we can make this an internal label recognized by cortex and then the translators can just import the label. No need to do this now but could be a great idea when there are other translators available.
- I prefer
source
overformat
because to me its more natural to refer to influx as a source database rather than a metric format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since this is going to be an important tag, we should define the string as a constant somewhere rather than as an inline literal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was part of the offline discussion before the initial comment was made, but thinking about it more, I think __proxy_source__
would be better. original
doesn't add much information to the label name, but source
by itself seems too generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think proxy_source is good. I will update it.
e28848e
to
74da19d
Compare
Go coverage report:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Go coverage report:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__proxy_source__
ftw
This PR adds an internal label to all metrics parsed by the proxy before being written to cortex. This internal label will be used for tracking the number of active series.