-
Notifications
You must be signed in to change notification settings - Fork 47
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
Ignore metrics by type #238
Conversation
Superseeds #235 (just for tracking pourposes) |
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.
Hello @smcavallo,
Let me explain my train of thoughts to explain it properly. I have created various config files that are targeting a Prometheus endpoint (an empty CoreDNS):
A file called filter-none
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
A file called filter-prefix
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
transformations:
- ignore_metrics:
- prefixes:
- go_th
A file called filter-suffix-info
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
transformations:
- ignore_metrics:
- suffixes:
- _info
A file called filter-suffix-bucket
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
transformations:
- ignore_metrics:
- suffixes:
- _bucket
A file called filter-type-count
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
transformations:
- ignore_metrics:
- metric_types:
- count
A file called filter-type-summary
:
standalone: false
emitters: infra-sdk
cluster_name: "non-empty"
targets:
- urls: ["http://localhost:9253"]
verbose: true
transformations:
- ignore_metrics:
- metric_types:
- summary
Then I run this shell oneliner (from your origin) to see if it filters correctly:
for i in filter-*; do go run ./cmd/nri-prometheus -configfile $i | jq '.data[].metrics|=sort_by(.name) | [ .data[].metrics[] | { "name": .name, "type": .type, "value": .value } ]' > out-$i.json; done
So I diff out-filter-none.json out-filter-prefix.json
and diff out-filter-none.json out-filter-suffix-info.json
. Here is the first diff:
# More values changing...
190c190
< "value": 53286
---
> "value": 53780
236,240d235
< },
< {
< "name": "go_threads",
< "type": "gauge",
< "value": 18
Here is the second diff:
# More values changing...
110,115c105
< "value": 16
< },
< {
< "name": "go_info",
< "type": "gauge",
< "value": 1
---
> "value": 17
# More values changing...
They look cool! Congrats.
But, if I diff out-filter-none.json out-filter-suffix-bucket.json
the diff is empty (aside values of the metrics changing).
% diff out-filter-none.json out-filter-suffix-bucket.json
% diff out-filter-none.json out-filter-suffix-bucket.json | wc -l
0
The _bucket
part is added by Prometheus is not part of the metric name so cannot be filtered there. That is the reason we are not supporting suffixes as they lead to errors like this. Is this your intended way?
On the other hand the second commit of this PR to filter by metric type is working perfectly:
diff out-filter-none.json out-filter-type-count.json
7,14d6
< "name": "coredns_forward_healthcheck_broken_total",
< "type": "cumulative-count"
< },
< {
< "name": "coredns_forward_max_concurrent_rejects_total",
< "type": "cumulative-count"
< },
< {
# More metrics being correctly filtered...
diff out-filter-none.json out-filter-type-summary.json
43,46d42
< "name": "go_gc_duration_seconds",
< "type": "prometheus-summary"
< },
< {
We can accept this PR if it only filters by metric_type
and it would be really useful.
Have a nice day!
3dba9e3
to
f719637
Compare
f719637
to
d4ded2c
Compare
b27a9da
to
d4ded2c
Compare
Signed-off-by: smcavallo <smcavallo@hotmail.com>
@kang-makes - I have made the requested updates for this PR. |
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.
Thank you very much for your patience on this contribution.
@kang-makes - thank you so much for reviewing and merging! It is most apparent when attempting to use suffix filtering because prometheus appends suffixes to these metric types. But it is also an issue for prefixes - just a case which rarely gets hit. this prefix rule will NOT work:
this exclude rule will NOT work:
Both of the above feel like bugs but they are WAD. The documentation could be made more clear there. It is confusing but it is the same issue with suffixes. This ability to use this suffix filter code would be useful for us
it is just extremely confusing to use suffix filters and makes it feel like metric filtering is broken. |
Hi @smcavallo, When we rejected your first PR @gsanchezgavier added a commit to the docs where it is explained that we filter by the We are struggling on how to create a better You are not the only one to hit this issue and put transformations:
- ignore_metrics:
- prefixes:
- coredns_
except:
- coredns_build_info
- prefixes:
- go_
except:
- go_threads Same command as above [
{
"name": "coredns_build_info",
"type": "gauge"
},
{
"name": "go_threads",
"type": "gauge"
}
] But, even putting the Nevertheless, I will have this issue in mind. |
allow metric filtering by metric type
Ex.
The use case is the ability to ignore histograms which generate a large number of (unused) metrics.
In the above case since
# TYPE
ishistogram
the above metrics will be filtered out.Except
lists can be used for histograms which should be kept.Signed-off-by: smcavallo smcavallo@hotmail.com