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

Replace exometer with prometheus #153

Closed
wants to merge 2 commits into from

Conversation

0xAX
Copy link
Member

@0xAX 0xAX commented Sep 1, 2020

These commits replaces exometer/exometer_core libraries with prometheus.erl to collect RADIUS metrics.

All metrics names are preserved as it was before. Only internal structure of metrics names changed as exometer stored metrics names as list of atoms, but prometheus stores metrics names as binary strings. In addition variadic parts of metrics names moved to prometheus labels.

So before it was:

[request, invalid, server, $NAME, $IP, $PORT, total, undefined, undefined, gauge]

Now it is:

request_invalid_server_total_undefined_undefined_gauge

with [$NAME, $IP, $PORT] labels.

The one backward compatibility change is - prometheus histogram observations are stored into buckets, but exometer histograms collected data and calculated quantiles. Now calculation of quantiles should be done on prometheus site via prometheus functions.

This commit replaces exometer/exometer_core libraries with prometheus.erl
to collect RADIUS metrics.

All metrics names are preserved as it was before. Only internal structure
of metrics names changed as exometer stored metrics names as list of atoms,
but prometheus stores metrics names as binary strings.

The one backward compatibility change is - prometheus histogram observations
are stored into buckets, but exometer histograms collected data and
calculated quantiles. Now calculation of quantiles should be
done on prometheus site via prometheus functions.
Copy link
Member

@RoadRunnr RoadRunnr left a comment

Choose a reason for hiding this comment

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

After reading all those changes, I'm seriously annoyed by the way the exometer stuff works.
I wonder if it wouldn't be cleaner and simpler to:

  • rip out/revert all the exometer support
  • rewrite this as a prometheus exporter reusing the existing SNMP and eradius_counter stuff

The counter collector might need some enhancements, but it looks much more modular and less intrusive that the exometer crap.

@mgumz comments?

update_client_request(retransmission, {{CName, CIP, undefined}, {SName, SIP, SPort}}, Ms) ->
update_request(client, retransmission, {CName, CIP, SName, SIP, SPort}, Ms);
update_client_request("pending", {{CName, CIP, undefined}, {SName, SIP, SPort}}, Pending) ->
update_request(client, "pending", {CName, CIP, SName, SIP, SPort}, Pending);
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this excessive replacing ot atoms with strings. The conversion can always done later when generating the metrics name.

| request_invalid_server_total_undefined_undefined_counter | [$NAME, $IP, $PORT] | counter |
| request_malformed_server_total_undefined_undefined_gauge | [$NAME, $IP, $PORT] | histogram |
| request_malformed_server_total_undefined_undefined_counter | [$NAME, $IP, $PORT] | counter |
| request_dropped_server_total_undefined_undefined_gauge | [$NAME, $IP, $PORT] | histogram |
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the effort to keep the existing names, but prometheus has it's own idea what metric names should look like and I think we should follow their lead (@mgumz what's you take on this?)

see: https://prometheus.io/docs/practices/naming/

@0xAX
Copy link
Member Author

0xAX commented Sep 25, 2020

I have pushed branch - https://github.com/0xAX/eradius/tree/prom-collector with prometheus collector based on eradius_counter things (most of all the things goes here https://github.com/0xAX/eradius/blob/prom-collector/src/eradius_prometheus_collector.erl), should be similar to what prometheus_diameter_collector does.

I have met two issues with this approach:

  1. As metrics are stored in eradius_prometheus_collector/eradius_counter now - usual prometheus.erl API for fetching of metrics like prometheus_counter:value and so on does not work anymore. This is probably good for eradius itself, but not good for other projects where we use eradius as it provides API to fetch metrics by name/labels. Metrics could be requested only via formatters and there is no possiblity to fetch metric by name/labels, only all the metrics in text/protobuf format at once.

The possible solution here could be not just return metric (https://github.com/0xAX/eradius/blob/prom-collector/src/eradius_prometheus_collector.erl#L102) from collector but return nothing from it and just create/update a metric via usual collectors like prometheus_counter, prometheus_gauge and so on.

  1. The second issue is histograms. We can't create histogram easily like for counters or gauges (for example here https://github.com/travelping/prometheus_diameter_collector/blob/master/src/prometheus_diameter_collector.erl#L59). prometheus.erl provides API for histograms https://github.com/deadtrickster/prometheus.erl/blob/master/src/model/prometheus_model_helpers.erl#L283 but as you may see we should provide buckets, sums and count which is possibly not the best way to calculate inside eradius. There is a solution for this - we can create histograms and use standard prometheus_histogram collector for this and not to use custom eradius collector. In this case histograms will be accessible via prometheus formatter and via standard api like prometheus_histogram:observe.

@RoadRunnr @mgumz What do you think about this?

@0xAX
Copy link
Member Author

0xAX commented Sep 29, 2020

going to close this PR as new one will be opened

@0xAX 0xAX closed this Sep 29, 2020
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.

None yet

2 participants