-
Notifications
You must be signed in to change notification settings - Fork 172
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
added extraction of labels from JSON column #37
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for writing the code. I'm not going to accept the PR, because I believe there are easier ways around it, plus a number of other reasons.
First, as you put it, JSON is a first class citizen in most databases nowadays, so you should be able to extract the label values in your query, just as you would select values from a regular table column.
Second, if the argument is that you can't have fully dynamic labels for your metrics, that is by design. Your data should not control what is essentially your schema. E.g. you don't want to have a metric with 3 labels and another metric, with the same name, but 4 labels. (I seem to recall that the Prometheus client library would blow up if you try to do that.) Labels should be configuration controlled, not data controlled.
That being said, I did add some comments to clean up the code. And I'll leave the PR around, in case someone else is interested.
But ok, thanks for your review. |
Hi, you might not like it, but I've implemented it works ;-)
This change introduces new metric option json_labels that specified a column that should be used to extract additional labels to metric, without the need to name them one by one in the query.
If some style/whatever changes needed to merge it, I'll make them.