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

added extraction of labels from JSON column #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

samsk
Copy link

@samsk samsk commented Jul 11, 2019

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.

Copy link
Owner

@free free left a 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.

metric.go Show resolved Hide resolved
metric.go Show resolved Hide resolved
metric.go Outdated Show resolved Hide resolved
metric.go Outdated Show resolved Hide resolved
config/config.go Show resolved Hide resolved
@samsk
Copy link
Author

samsk commented Jul 16, 2019

  1. yes you can, but than you have to change configuration. I kind a like it, when everything about database is declared in database, even what is monitored and what labels it has - ie. if there is bool metric (ok/down), than the evaluation logic should be in database view and not in the query in some config file - and if it is so, why not to extend this logic to labels ;-)

  2. Regarding fully dynamic labels - I know that prometheus will refuse different labels for the same metric and that is correct and expected. They should not be absolutely dynamic.

But ok, thanks for your review.

metric.go Outdated Show resolved Hide resolved
metric.go Show resolved Hide resolved
metric.go Show resolved Hide resolved
metric.go Show resolved Hide resolved
metric.go Outdated Show resolved Hide resolved
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