python collectd prometheus scraper

Question

I've been tinkering with python for quite a while but have never taken on a challenge like this - I'm trying to write a collectd plugin to scrape metrics from prometheus endpoints.

My goals are to make it as stable as possible - i'd like to put this into production when i am happy with it. There are a couple of items that i haven't figured out yet (summary and histogram familytypes), so if anyone has any suggestions about how to do that, i would be very appreciative. As well as this, i am trying to be a pythonic as possible, but i am 100% self taught, so my code is probably all over the place.

I'm interested in any form of feedback, even if it means i need to go learn/un-learm some more stuff :)

import platform
import collectd
from prometheus_client.parser import text_string_to_metric_families
import requests

def config_func(config):
    endpoint_set = False
    endpoint_set = False
    interval = 60

    for node in config.children:
        key = node.key.lower()
        val = node.values[0]

        if key.lower() == 'endpoint':
            endpoint = val
            endpoint_set = True
        elif key.lower() == 'endpoint_name':
            endpoint_name = val
            endpoint_name_set = True
        elif key.lower() == 'interval':
            interval = val
        else:
            collectd.info(f'prometheus_scraper: Unknown config key "{key}"')


    if endpoint_set and endpoint_name_set:
        collectd.info(f'prometheus_scraper: python version {platform.python_version()}')
        collectd.info(f'prometheus_scraper: Using {endpoint} for {endpoint_name} on ')

        global ENDPOINT
        ENDPOINT = endpoint
        global ENDPOINT_NAME
        ENDPOINT_NAME = endpoint_name
        global INTERVAL
        INTERVAL = interval


def parse_func(metrics):

    for family in text_string_to_metric_families(metrics):
        for sample in family.samples:

            if family.type == 'summary':
                #print(family.type)
                pass
            elif family.type == 'histogram':
                #print(family.type)
                pass
            else:
                val = collectd.Values()
                val.plugin = ENDPOINT_NAME
                val.interval = INTERVAL
                val.type = family.type

                if len(sample.labels) > 0:
                    joined = ''.join(key + '_' + str(val) for key, val in sample.labels.items())
                    val.type_instance = sample.name + '.' + joined
                else:
                    val.type_instance = sample.name

                #print(f'type_instance: {val.type_instance}')

                val.values = [sample.value]
                val.dispatch()


def read_func():

    try:
        metrics = requests.get(url=ENDPOINT)
        metrics.raise_for_status()
        parse_func(metrics.text)
    except requests.exceptions.RequestException as e:
        collectd.error(f'failed to retrive metrics: {e}')


collectd.register_config(config_func)
collectd.register_read(read_func, INTERVAL)

https://github.com/mark-vandenbos/collectd_prometheus_scraper/

Answer 1

I can't test this code, so I will only make a few general remarks for the time being.

I was curious, since I actually started working on the Prometheus API yesterday, because I was asked to design a monitoring dashboard.

---

This code is un-Pythonic:

    global ENDPOINT
    ENDPOINT = endpoint
    global ENDPOINT_NAME
    ENDPOINT_NAME = endpoint_name
    global INTERVAL
    INTERVAL = interval

Assigning a variable to another variable like this: ENDPOINT = endpoint has no added value and is even confusing. All this is redundant.

Global variables are frowned upon, because they are seldom necessary in fact. There usually is a better way. Just pass the variables along to your functions and retrieve resulting values as necessary.

If you have constants, define them at the top of your code (if you have just one file), or put them in a separate config file. I suggest that you create a config.py file, and move all your constants there. Then you import the file:

import config

And you can refer to your constants like this all across your code:

config.ENDPOINT

Source and there are more useful tips too. This is recommended reading for beginners and more experienced programmers as well.

---

You have this code:

        if family.type == 'summary':
            #print(family.type)
            pass
        elif family.type == 'histogram':
            #print(family.type)
            pass
        else:

pass is no operation (NOP). So you have code that does strictly nothing and should be removed. What you want is probably something like this:

if family.type not in ('summary', 'histogram'):

---

You have 3 functions: config_func, parse_func, read_func. It's obvious they are functions, so the _func suffix is superfluous. But the names are not descriptive enough. It doesn't tell me what you are reading or parsing. So I think your naming conventions should be revisited.

In a way I think the order of things is logical: configuration, reading results, parsing results. Gotta make it more obvious.

---

Something that is lacking badly: comments. They are not just for external reviewers but for your benefit too. After a while you will forget stuff and you will have to re-analyze your own code to figure out the flow and what kind of data is returned by a given function. I would recommended that you document each function with a docstring. 2-3 lines will do.

Document your functions, and describe what kind of data you are manipulating. When you have some ifs and are testing for a specific condition, it does not hurt to explain why in a comment, unless the purpose of the test is very obvious.

Since you put your code on Github there is an expectation that somebody else might want to download it, use it, possibly fork it. But then it is important to provide enough documentation. Lack of documentation can be forgiven for software that works straight out of the box without any configuration, but this is rare.

---

Quality control: in config_func (at the top of your code) you have repeated code:

endpoint_set = False
endpoint_set = False

A good IDE would help you spot those mistakes and also enforce PEP8 rules. In a job interview this would qualify as lack of attention to detail, just by proof-reading your code carefully you should have noticed :)

---

In a Python program it is customary to have a main method and use __main__ like this:

if __name__ == '__main__':
    collectd.register_config(config_func)
    collectd.register_read(read_func, INTERVAL)

Or better yet:

if __name__ == '__main__':
    main()

And put your launcher code in the main function. The benefit of doing so is that you can import your package (to reuse some functions) without actually launching it.

---

Now that I am reading what I wrote so far I realize that it's not very helpful. I only gave you general suggestions but I think they are still useful though. But I am kinda frustrated because you have not described what the finished product does look like. After reading your code I can't really figure it out. I don't have a Prometheus box handy so I can't run your code and pull out some data.

The code is quite short (< 100 lines) and yet it is obscure to me. I don't think it's only because I am Prometheus newbie. The code does not speak for itself. Comments would help make more sense of the overall approach.

I am not familiar with collectd so I can't comment much on that. I understand you are scraping metrics but what is the ultimate goal, how will the data be used, that's what is interesting.