feat: Add Prometheus metrics for data integrity checks

[jason-p-pickering]

No description provided.

[luciano-fiandesio]

One thing that I would consider doing, since we are probably going to add more of these metrics is to use constants for string like gauge, percentage , count. We can create a MetricsConstants class or something like that, so we avoid repeating these "technical" strings. What do you think?

[jbee]

We can create a MetricsConstants class or something like that, so we avoid repeating these "technical" strings. What do you think?

Actually, I am strongly against such constants. Syntax should not be extracted to constants. It exists to indicate meaning. Replacing it with an alias is just making the brain go one extra step, to recompose the concatenation of building blocks back into the string that creates. I think the way to go for syntax is always using a template.

To illustrate the point, a few versions of a JSON5 response

return "{'id':'%s'}".formatted(id);

return "{'"+ID+"'"+COLON+"'"+id+"'}";

return OBJ_OPEN+QUOTE+ID+QUOTE+COLON+QUOTE+id+QUOTE+OBJ_CLOSE;

I hope it is clear that the more constants you use the less you can see what the result is.

I know we still have many such syntax and name constants in the codebase but that is not to set a good example but just because at some point people it was considered "bad" to to inline constants so they got extracted to named constants without without making a difference between constants that need an explanation of why that number or string is the right one to use and constants everyone knows why they are used because they are names belonging to standards we are familiar with. We just didn't get to cleaning this up - and it remains a difficult topic because there is no easy rule to follow.

[jbee]

As I explained in my other comment, I think these belong in string templates. So, one way to do that would be to have dedicated methods in PrometheusTextBuilder, like gaugeLine which would be similar to typeLine just that the type is already defined by the method called. Or just put them inline as an argument string to helpLine, like, metrics.typeLine("dhis_data_integrity_check", "gauge" );. Which as you see I would also do for the name. the goal with things like this is always to make the code read like the result. And the result is: # TYPE dhis_data_integrity_check gauge. So personally, I'd even go as far as naming the method TYPE, so
metrics.TYPE("dhis_data_integrity_check", "gauge"); If we compare code and result

metrics.TYPE("dhis_data_integrity_check", "gauge");
#TYPE dhis_data_integrity_check gauge

To me that is a compelling reason. If it reads like what you want to create chances are it will just do that correctly. Everything that adds clutter and translation to it just taxes you brain into making mistakes.
As I explained, in that regard I don't think that general coding rules and conversions should apply, like not naming a method TYPE because of camel-case is the norm. Or not repeating and inlining constants.

[jason-p-pickering]

Thanks for the feedback @jbee . In the last commit, I have removed the constants class, and tried to simplify things a bit inside the class. For now, the only type of metrics we have is a "gauge", and considering how difficult it is for me to spell it, a constant helps in this case. :-D I did add some default methods in the PrometheusTextBuilder though, which makes by default, the metric type a gauge. I think this makes sense (at least for now) since all of the metrics we are exposing are this type. I also renamed the methods per your example.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud