node-exporter-textfile-collector-scripts icon indicating copy to clipboard operation
node-exporter-textfile-collector-scripts copied to clipboard

Add Zypper updates and patches script metrics

Open Wabri opened this issue 1 year ago • 12 comments

Wabri avatar Oct 14 '24 13:10 Wabri

I notice that several of the metrics are specified as counters, but they will presumably fluctuate up and down as the number of available updates ebbs and flows. As such, they should be designated as gauge, not counter.

Whilst this repo accepts both shell scripts and Python scripts, once a shell script starts to include non-trivial amounts of awk, I think my personal feeling would be that it's time to write it in Python - where you can leverage the official Prometheus Python client library.

dswarbrick avatar Oct 18 '24 16:10 dswarbrick

Ok, I'll create the python script so we can compare what are the most complex to maintain or read :+1:

Wabri avatar Oct 19 '24 16:10 Wabri

Hi @dswarbrick I've transcript the basch script into python script. Let us know what do you think about it!

Wabri avatar Oct 23 '24 09:10 Wabri

@Wabri I won't take a serious look at this PR until the lint CI job is green. However, you seem to have overlooked my comment where I said that Python scripts can utilize the official Prometheus Python client library. This was more than just a suggestion - it's a requirement for Python scripts in this repo. It avoids the need for people to (re)invent their own text exposition format functions, which are often incomplete or buggy.

dswarbrick avatar Oct 23 '24 12:10 dswarbrick

@dswarbrick let me know if there is anything to change in the code using the prometheus python client library.

Wabri avatar Oct 25 '24 15:10 Wabri

@Wabri There seems to be some miscommunication here, because your Python script does not make use of the official Prometheus Python client library at all.

Let me be clear. Python code such as this will not be accepted by this repo:

    print('# HELP zypper_patches_pending_reboot_total zypper patches available which require '
          'reboot total')
    print('# TYPE zypper_patches_pending_reboot_total counter')
    print_patches_sum(data_zypper_lp,
                      prefix="zypper_patches_pending_reboot_total",
                      filters={'Interactive': 'reboot'})

The rendering of text exposition format metrics is handled by the client_python library, avoiding the need for users to implement their own (often incomplete and / or buggy) methods.

Documentation for client_python can be found at https://prometheus.github.io/client_python/, and you can also look at the existing Python scripts in this repo for some examples of how to use it.

Jumping ahead a little bit, I also see that there is no error checking or handling being performed when calling the /usr/bin/zypper external process. It would be nice to see something added there, e.g. handle command execution failure gracefully, output error (to stderr) and exit with non-zero status.

dswarbrick avatar Oct 25 '24 17:10 dswarbrick

No miscommunication, I understand that you want the library in my python script, but I've never use it and in my previous comment I was just asking where do I need to use it. I saw that the library is lacking of documentation, for me at least.

Thanks for the clarification, I'll go ahead with the changes you ask to do and I'll be back.

Wabri avatar Oct 27 '24 08:10 Wabri

Hi @dswarbrick!

I've done some changes:

  • I removed all the non standard information print by replacing them with the prometheus client library functions
  • Add the subprocess error handling with the output of stderr
  • Refactor the extraction of the zypper list datas to let the code be more readable

I've not tested the scripts yet, I'll set the pr ready for review when I've done it!

Wabri avatar Oct 27 '24 17:10 Wabri

I saw that the library is lacking of documentation, for me at least.

I literally posted a link to the client_python documentation in my previous comment: https://prometheus.github.io/client_python/

dswarbrick avatar Oct 27 '24 20:10 dswarbrick

You are right, I meant lacking "a good" documentation. But it is a personal thought.

Wabri avatar Oct 27 '24 20:10 Wabri

@dswarbrick tested and it works as expected!

Wabri avatar Nov 04 '24 12:11 Wabri

You are right, I meant lacking "a good" documentation. But it is a personal thought.

I agree, the online HTML documentation is a bit sparse, mostly only showing example usage. However, client_python, like most Python code, is effectively self-documenting. For example:

>>> from prometheus_client import Gauge
>>> help(Gauge)
Help on class Gauge in module prometheus_client.metrics:

class Gauge(MetricWrapperBase)
 |  Gauge(name: str, documentation: str, labelnames: Iterable[str] = (), namespace: str = '', subsystem: str = '', unit: str = '', registry: Optional[prometheus_client.registry.CollectorRegistry] = <prometheus_client.registry.CollectorRegistry object at 0x7c1844c2d5b0>, _labelvalues: Optional[Sequence[str]] = None, multiprocess_mode: Literal['all', 'liveall', 'min', 'livemin', 'max', 'livemax', 'sum', 'livesum', 'mostrecent', 'livemostrecent'] = 'all')

dswarbrick avatar Nov 04 '24 20:11 dswarbrick