[IMPROVEMENT] Added exporter labels. #381

ekhvalov · 2022-01-22T02:12:37Z

To resolve issues #346 and partially #180

kfdm

Overall, a good start! You covered a lot of the basic functionality and even added tests!!!

Part of the challenge for this (and Promgen overall) is how to make it easy, to modify a complicated configuration. There are lots of edge cases to consider. One that I quickly noticed, is that while you call out __param_ labels (good!) they're not actually taken into account when using the Exporter text button, which could be quite confusing when Promgen and Prometheus scrape things differently.

That combined with the other comment about displaying the custom labels (and not just using title= attribute, I think we still need to think through the exact workflow a little more.

I think my next two days will be a little busy, but hopefully that gives you some good feedback. I'll try to circle back to this PR in a few days.

kfdm · 2022-01-24T08:43:05Z

promgen/templates/promgen/exporter_form.html

+        {{ labels_formset.management_form }}
+        <div class="panel panel-default">
+            <div class="panel-heading" role="tab" id="labels-formset-panel-header">
+                <a role="button" data-toggle="collapse" href="#labels-formset-panel" aria-expanded="false"


Thinking back and forth a bit about this. It's probably good to have it toggle visible like you have it, but I'm wondering if we may need to make small adjustments to the text descriptions

What adjustments you want to make?

The main issue for me is the scrape exporter code. First of all there is a TODO in the code. Probably API /rest/project/<slug>/scrape shoult know nothing about exporter register form fields and should receive a POST with body like {"host":"blackbox", "port":9115, "scheme": "https", "path": "/probe", "query": {"target": "prometheus.io"}}. If so, then ExporterScrape class could build the POST body from register exporter form then query API and return API response. Is that OK?
Maybe it is better to add a field is_parameter to ExporterLabel? In that case user is no need any more to use a prefix __param_ and all logic about prefix could be moved to prometheus render_config function.
Also I have a question about API /rest/project/<slug>/scrape. Why exporter's id is omitted? The url could be /rest/project/<slug>/exporter/<slug>/scrape. In that case all url logic could be encapsulated in an Exporter itself.

The reason the scrape code is as is, is because you can use the test button before it has been saved to the database, so we need to be able to POST the data to a test input when we don't already have an exporter_id.

kfdm · 2022-01-24T08:44:32Z

promgen/templates/promgen/project_detail_exporters.html

@@ -10,7 +10,9 @@
      <th colspan="4">Actions</th>
    </tr>
  {% for exporter in project.exporter_set.all %}
-    <tr {% if not exporter.enabled %}class="promgen-disabled"{% endif %}>
+    <tr {% if not exporter.enabled %}class="promgen-disabled"{% endif %}


Even though we use the title= attribute in a few places to show extra information, I think in this case, additional labels are quite important, and we need to figure out how to make them always visible. Part of the challenge though is we do not currently have a lot of space there, so still thinking if we need to have a toggle collapse menu for it or not.

Will button Labels be OK? Button

Button with dropdown table added.

kfdm · 2022-01-24T08:46:55Z

promgen/tests/test_prometheus.py

+    def setUp(self, mock_signal):
+        user_id = 999
+        user = self.add_force_login(id=user_id, username='Tester')
+        self.__create_services([


Instead of having all this structure in code, we should really be using fixtures for the tests. (Older, existing tests should also be migrated to fixtures but that's something I should clean up)

https://docs.djangoproject.com/en/4.0/howto/initial-data/

OK, I'll use fixtures.

ekhvalov · 2022-01-27T02:29:18Z

New exporter scraping is fixed but scraping for existed exporters is still not working. We have to decide how to deal with it. I think we have two different types of scraping, one for existed exporters and another for new ones. Probably we have to scrape new exporters using my solution and create API especially for existed exporters.
Also I have some questions.
If is_parameter field was added, is it correct to allow users to name labels with double underscore prefix?
Any validation is needed?

kfdm

Moving some of the data to fixtures helps clean up the tests a lot. This is great!
I don't think we want to depend on a is_parameter though, since I think that will add a good deal of confusion.

I did a quick pass today, but I will likely have to wait until next week to do a more detailed pass. I have a few tasks that will likely take the rest of my attention this week. Thank you for your patience 🙇

kfdm · 2022-01-27T05:26:58Z

promgen/forms.py

+        widgets = {
+            'name': forms.TextInput(attrs={'class': 'form-control', 'placeholder': 'Name'}),
+            'value': forms.TextInput(attrs={'class': 'form-control', 'placeholder': 'Value'}),
+            'is_parameter': forms.CheckboxInput()


I think we don't want to use a parameter here (and then later build a string). I think having this as a toggle, and then try to generate strings from this later is confusing. If a user inputs a label as foo or __param_foo or __invalid_foo then that should be the exact name they see, and the exact name that is written into the Prometheus target.
We do however need to make sure we pass along the parameter when someone clicks the test version so we'll likely need to filter on that.

# psudo code params = {} for key in labels: if key.startswith('__param'): params[ key[:7] ] = labels[key]

I think this will cause the least amount of confusion in the long run.

kfdm · 2022-01-27T05:31:46Z

promgen/forms.py

+                continue
+            if form.cleaned_data.get('is_parameter'):
+                if name in parameters_names:
+                    e = ValidationError('Duplicated name', code='invalid')


I think I need to try to test this on my own, but something seems a bit off at first glance. I think all the checks for duplicated can probably just be left to the database and you shouldn't need custom logic in here.

kfdm · 2022-01-27T05:32:38Z

promgen/models.py

+    value = models.CharField(
+        max_length=128,
+    )
+    is_parameter = models.BooleanField(default=False)


as mentioned above, I think having a toggle for this is going to be more confusing to the user, than doing a bit of filtering on key.startswith('__param')

kfdm · 2022-01-27T05:33:04Z

promgen/prometheus.py

@@ -132,12 +132,21 @@ def render_config(service=None, project=None):
        if exporter.path:
            labels["__metrics_path__"] = exporter.path

+        for label in exporter.exporterlabel_set.all():
+            if label.name not in labels:
+                label_name = '__param_{}'.format(label.name) if label.is_parameter else label.name


As mentioned above, I think we don't want to use is_parameter but filter on __param_ instead.

kfdm · 2022-01-27T05:33:54Z

promgen/templates/promgen/exporter_form.html

+        {{ labels_formset.management_form }}
+        <div class="panel panel-default">
+            <div class="panel-heading" role="tab" id="labels-formset-panel-header">
+                <a role="button" data-toggle="collapse" href="#labels-formset-panel" aria-expanded="false"


The reason the scrape code is as is, is because you can use the test button before it has been saved to the database, so we need to be able to POST the data to a test input when we don't already have an exporter_id.

kfdm · 2022-01-27T05:35:51Z

promgen/tests/test_prometheus.py

+
+    def test_create_config(self):
+        config = prometheus.create_config()
+        self.assertEqual(config, [


Thanks!! Moving most of it to a filter really helps clean up the code. I think this config though can also be cleaned up a bit. You should be able to use our help function to put these test configurations as json as well

TEST_CONFIG = tests.Data('examples', 'test-targets.json').json()

or something similar

ekhvalov · 2022-01-31T06:06:29Z

I have removed field is_parameter and moved test config data to separate file in the examples folder.
There is one more thing I have to do, is to solve saved exporters testing. I need some time to do it, because I have to get familiar with rest-framework.

ekhvalov · 2022-02-12T10:43:18Z

Exporters scraping is working via new REST endpoint. The old endpoint is still exists and could be removed if new one is OK.

kfdm · 2022-03-31T01:01:14Z

I apologize for the long delay without comment. I have not forgotten this PR, I have just been preoccupied with other work tasks which have made it more difficult to prioritize this.

Advanced users may have specific, legitimate use cases for this, there is also a large set of less advanced users who will not understand the implications and are very likely to make things more complicated by adding additional custom labels. I'm somewhat reluctant to merge as is, again not because the idea or code are bad, but because we need to be very careful that we can clearly communicate to the user when this feature should be used (since it likely should not be used in the most common cases). Unfortunately, this April may also be a little bit scattered in terms of ability so I can not currently commit to when I'll be able to think through all the implications of this change. 🙇

ekhvalov added 2 commits January 22, 2022 11:00

Added exporter labels.

f302b1c

Merge remote-tracking branch 'upstream/master' into exporter-labels

3e65471

kfdm self-requested a review January 24, 2022 02:46

kfdm self-assigned this Jan 24, 2022

Merge remote-tracking branch 'upstream/master' into exporter-labels

bcf3da0

ekhvalov requested a review from a team as a code owner January 24, 2022 03:56

kfdm requested changes Jan 24, 2022

View reviewed changes

ekhvalov added 8 commits January 25, 2022 13:33

Field 'is_parameter' added to ExporterLabel model.

35a5ec5

Merge remote-tracking branch 'upstream/master' into exporter-labels

1d095c0

Field 'is_parameter' added to ExporterLabel model.

2b5aed2

Dropdown labels list added to exporters panel.

c380dad

Merge remote-tracking branch 'upstream/master' into exporter-labels

1b0fb5d

Indentation fixed.

3622f7b

Test fixtures used.

425402a

Fixed new Exporter scraping.

8dc3649

kfdm requested changes Jan 27, 2022

View reviewed changes

ekhvalov added 2 commits January 31, 2022 13:02

Expected test results are moved to separate file.

9f68901

Field 'is_parameter' is removed.

2221014

ekhvalov added 2 commits February 11, 2022 12:12

Merge remote-tracking branch 'upstream/master' into exporter-labels

7573f09

Implemented REST endpoint '/rest/project/<slug>/scrape'.

9dd046d

ekhvalov closed this Apr 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMPROVEMENT] Added exporter labels. #381

[IMPROVEMENT] Added exporter labels. #381

ekhvalov commented Jan 22, 2022

kfdm left a comment

kfdm Jan 24, 2022

ekhvalov Jan 25, 2022

ekhvalov Jan 25, 2022

kfdm Jan 27, 2022

kfdm Jan 24, 2022

ekhvalov Jan 25, 2022

ekhvalov Jan 27, 2022

kfdm Jan 24, 2022

ekhvalov Jan 25, 2022

ekhvalov Jan 27, 2022

ekhvalov commented Jan 27, 2022

kfdm left a comment

kfdm Jan 27, 2022

kfdm Jan 27, 2022

kfdm Jan 27, 2022

kfdm Jan 27, 2022

kfdm Jan 27, 2022

kfdm Jan 27, 2022

ekhvalov commented Jan 31, 2022 •

edited

Loading

ekhvalov commented Feb 12, 2022

kfdm commented Mar 31, 2022

[IMPROVEMENT] Added exporter labels. #381

[IMPROVEMENT] Added exporter labels. #381

Conversation

ekhvalov commented Jan 22, 2022

kfdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekhvalov commented Jan 27, 2022

kfdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekhvalov commented Jan 31, 2022 • edited Loading

ekhvalov commented Feb 12, 2022

kfdm commented Mar 31, 2022

ekhvalov commented Jan 31, 2022 •

edited

Loading