3786 Add OpenMetrics (Prometheus) statistics endpoint

[hacklschorsch]

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3786

[exarkun]

Thanks! Some comments left inline.

[hacklschorsch]

Uff

CI Problemo:

Expected to find no classic classes.

[...]

class HackItResource(Resource):

Is this a classic class?

[exarkun]

Uff

CI Problemo:

Expected to find no classic classes.

[...]

class HackItResource(Resource):

Is this a classic class?

I guess it is because twisted.web.resource.Resource is. Mix in object (HackItResource(Resource, object)) et voila...

[hacklschorsch]

class HackItResource(Resource):

Is this a classic class?

I guess it is because twisted.web.resource.Resource is. Mix in object (HackItResource(Resource, object)) et voila...

Ah thanks! I had hoped it was this easy :)

[hacklschorsch]

d'uh! I wanted to close the conversation thread, not the PR of course :(

[coveralls]

Coverage increased (+0.02%) to 95.499% when pulling 7183d53 on hacklschorsch:3786.openmetrics into bec813f on tahoe-lafs:master.

[hacklschorsch]

@exarkun please update the ticket status (the pending re-review) if you are good with this as it stands - I don't have the authority to do it.

[exarkun]

I like this now (also I am an author on some of it).

[meejah]

Overall looks good.

I find the test a little hard to follow .. since it only asserts a single thing in the end, maybe the "fake stats" could also just include that one thing? On the plus side, it does say how to re-create the huge dict. :)

Super minor nit, but could render_OPENMETRICS just be render_openmetrics or is there a need/reason for the all-uppercase?

[hacklschorsch]

Hi @meejah! Thank you for your review and feedback!

Super minor nit, but could render_OPENMETRICS just be render_openmetrics or is there a need/reason for the all-uppercase?

The other methods like that one are named similarly and this code makes it look like that specific part of the name should be upper cased.

[hacklschorsch]

since it only asserts a single thing in the end, maybe the "fake stats" could also just include that one thing?

The real check is that list(parser.text_string_to_metric_families(body)) doesn't raise an Exception, i.e. the OpenMetrics parser accepts our output as valid OpenMetrics.

The check for families[-1].name Equals("tahoe_stats_storage_server_total_bucket_count") is a mere safeguard to make sure the parser did make some output from its input - in other words, check against the output being a valid but empty string. (which wouldn't really be valid since it still needs the #EOF marker, but alas)

[exarkun]

FWIW I think the complexity of the test follows from the complexity of Tahoe's implementation of metrics collection. There is very little structure in the metrics Tahoe is gathering - mostly they are free-form strings associated with with a (probably) number. The test has no way to automatically collect all real metrics from the codebase (the only way to get them is to run Tahoe and make sure you hit every metrics-generating codepath).

A great future cleanup could be introducing some more structure in the metrics gathering code. This could have a specific goal (among others) of allowing tests to easily verify all metrics that are reported can be exported into the necessary format.

@meejah gave an approval along with this comment which I would interpret to mean "it would be nice to improve this area but it is not required before merging". I think we should go ahead and merge this with the complex test and as there is motivation to further Tahoe metrics improvements, we can incrementally improve the factoring and testing.

[meejah]

Yes, my comments (along with the "approve") was meant to mean, "here are things I was slightly confused about, update or not then merge". Hopefully I'm not screwing anything up by just merging this now since it seems ready-to-go...