Refactor Prometheus metric mappings #9948

exekias · 2019-01-08T12:50:37Z

This change refactors the way we store Prometheus metrics
to avoid the need for user defined mappings.

Metrics will be stored under fields with their original names, the sames
that users see in Prometheus. All metric fields will use double type,
which is the internal store type in Prometheus.

All labels are stored under prometheus.labels.* and mapped as keyword.

As it was already happening, metrics are grouped when they share the same exact
set of labels. Only difference is that quantile (summaries) and le (histograms) labels
have been put back in labels, that will end up in some more documents when
these metric types are present.

As fields/labels mappings cannot longer collide, we can get rid of
namespacing, which could lead to mapping explosion. Users can still use
custom fields to differentiate namespaces.

Stats metricset has been removed as it is no longer necessary, the
module can now scrape these metrics from Prometheus server to correctly
mapped fields.

Example output:

{
    "@timestamp": "2017-10-12T08:05:34.853Z",
    "agent": {
        "hostname": "host.example.com",
        "name": "host.example.com"
    },
    "event": {
        "dataset": "prometheus.collector",
        "duration": 115000,
        "module": "prometheus"
    },
    "metricset": {
        "name": "collector"
    },
    "prometheus": {
        "metrics": {
            "http_request_duration_microseconds_count": 0,
            "http_request_duration_microseconds_sum": 0,
            "http_request_size_bytes_count": 0,
            "http_request_size_bytes_sum": 0,
            "http_response_size_bytes_count": 0,
            "http_response_size_bytes_sum": 0
        },
        "labels": {
            "handler": "drop_series"
        }
    },
    "service": {
        "address": "172.29.0.2:9090",
        "type": "prometheus"
    }
}

closes #9921

cc @elastic/infrastructure

This change refactors the way we store Prometheus metrics to avoid the need for user defined mappings. Metrics will be stored under fields with their original names, the sames that users see in Prometheus. All metric fields will use `double` type, which is the internal store type in Prometheus. All labels are stored under `prometheus.labels.*` and mapped as keyword. As fields/labels mappings cannot longer collide, we can get rid of namespacing, which could lead to mapping explosion. Users can still use custom fields to differentiate namespaces. Stats metricset has been removed as it is no longer necessary, the module can now scrape these metrics from Prometheus server to correctly mapped fields.

houndci-bot · 2019-01-08T12:50:46Z

metricbeat/module/prometheus/collector/data.go

 }

-func GetPromEventsFromMetricFamily(mf *dto.MetricFamily) []PromEvent {
+func (p *PromEvent) LabelsHash() string {


exported method PromEvent.LabelsHash should have comment or be unexported

houndci-bot · 2019-01-08T12:50:46Z

metricbeat/module/prometheus/collector/collector.go

 	}, nil
 }

-func (m *MetricSet) Fetch() ([]common.MapStr, error) {
+func (m *MetricSet) Fetch(reporter mb.ReporterV2) {


exported method MetricSet.Fetch should have comment or be unexported

jsoriano

I like this 👍

Will we keep the mappings for the metricsets based in this one like the kubernetes ones?

jsoriano · 2019-01-08T19:09:16Z

metricbeat/module/prometheus/_meta/fields.yml

+        Prometheus metric labels
+    - name: prometheus.*
+      type: object
+      object_type: double


jsoriano · 2019-01-08T19:10:51Z

metricbeat/module/prometheus/_meta/fields.yml

  short_config: false
  release: ga
  settings: ["ssl", "http"]
  fields:
-    - name: prometheus
-      type: group
+    # Order is important here, labels will match first, the rest are double


What about placing labels in the base level to avoid possible issues with this? This is also the recommendation in ECS.

I would prefer to keep it like this for now, main reasons are:

It seems there is no way to do alias on labels.*

If other modules push labels there, it could lead to mapping issues if they are using dots

I want to make sure this module won't end on mapping issues and we can use these metrics reliably, not against this change once the problem is gone (for instance, if/when index per module is implemented).

WDYT?

LGTM, but I wonder some things (not blocking this PR)

It seems there is no way to do alias on labels.*

Why would we want to do it here?

If other modules push labels there, it could lead to mapping issues if they are using dots

I guess this is no different with any other module, maybe we shouldn't have a field for labels in ECS till we don't have a complete solution for these mapping issues.

I guess this is no different with any other module, maybe we shouldn't have a field for labels in ECS till we don't have a complete solution for these mapping issues.

That could make sense, I still have some doubts on how ECS will play out in cases like this, I would prefer to play it safe on this module and learn from the process with others 😇

jsoriano · 2019-01-08T19:12:06Z

metricbeat/module/prometheus/collector/collector.go

-	cfgwarn.Beta("The prometheus collector metricset is beta")
-
-	config := struct {
-		Namespace string `config:"namespace" validate:"required"`


exekias · 2019-01-09T09:56:05Z

About existing modules based on the Prometheus helper, this change should not affect them

ruflin · 2019-01-09T12:04:25Z

Can you elaborate on why this makes the stats metricset obsolete. I think for cases where we know the exact data structure we should still allow structured data?

exekias · 2019-01-09T14:10:29Z

stats metricset queries Prometheus server for its own metrics, it only retrieves 2 simple metrics. From what I see in our forums, many people get confused by the two metricsets, when normally what they want is the collector.

The collector will give you the two metrics anyway, so I'm inclined towards removing that complexity

jsoriano · 2019-01-10T20:48:43Z

What will be the migration path for someone using current module?

exekias · 2019-01-10T21:32:06Z

I didn't plan to offer a migration plan, as it's a breaking change. Users should be able to still consume both old and new metrics, they will just need different queries. Do you think we need more?

ruflin · 2019-01-11T12:38:00Z

@exekias I think the more general problem here is that we mix modules with the collection of data. stats for me is still a valid metricset as part of a module, collector is kind of a hack that we introduced as a metricset :-) When we compare this to Filebeat, this is more like an input.

Moving forward I think there are 2 options:

We keep treating inputs and modules/metricsets as the same thing. We have this also in http and others
We introduce the concept of an input.

I'm in favor of option 2 as I think it will remove the confusion that there are modules which are not really modules but not sure we can implement something like this on the short term.

roncohen · 2019-01-14T14:11:43Z

+1 on separating inputs and modules for metricbeat. That being said, this PR is a step in the right direction. Would be interesting to see if we can make the input/module separation for 7.0 as it seems it would be a breaking change.

exekias · 2019-01-14T15:56:41Z

Thank you everyone for your feedback, I understand we are good with the proposed change, I will go ahead and add missing docs/examples. Will ping you back when it's ready for review

exekias · 2019-01-22T14:26:33Z

docs updated, this is ready for a second round of review

ruflin

Left a few minor comments. Overall LGTM. Still not a big fan of removing the the stats metricset but we can add it later again if we see requests for it.

ruflin · 2019-01-22T14:59:56Z

metricbeat/docs/modules/prometheus.asciidoc

@@ -5,11 +5,9 @@ This file is generated! See scripts/docs_collector.py
 [[metricbeat-module-prometheus]]
 == Prometheus module

-This module periodically fetches metrics from
+This module periodically scrapes metrics from


As you remove the stats metricset, we could already be here more specific that it scrapes metrics from prometheus exporters?

ruflin · 2019-01-22T15:01:39Z

metricbeat/module/prometheus/collector/_meta/data.json

-            "prometheus_sd_kubernetes_events_total": {
-                "value": 0
-            }
+        "http_request_duration_microseconds_count": 0,


An alternative here would have been prometheus.metrics.* to not conflict with labels. Will also allow us to add more metadata in the future.

ruflin · 2019-01-22T15:02:11Z

metricbeat/module/prometheus/collector/_meta/data.json

+    },
+    "service": {
+        "address": "172.29.0.2:9090",
+        "type": "prometheus"


This is an interesting one. We should probably make it configurable so the user can set service.type and service.name.

ruflin · 2019-01-22T15:04:30Z

metricbeat/module/prometheus/collector/_meta/docs.asciidoc

+[source,yaml]
+-------------------------------------------------------------------------------------
+metricbeat.modules:
+- module: prometheus


This is kind of like a second "flawor" of the metricset. In the future we potentially could add a simple config federate.enabled: true which will set the correct settings directly.

I like it, it would be a nice addition, I agree it can be added later

ruflin · 2019-01-24T07:51:10Z

@exekias Will need a make update.

exekias · 2019-01-25T11:53:12Z

@ruflin I think this one is ready to go?

ruflin

Let's get this in. We can still do minor changes in follow up PR's.

ruflin · 2019-01-25T12:04:58Z

CHANGELOG.next.asciidoc

@@ -69,6 +69,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d

 *Metricbeat*

+- Refactor Prometheus metric mappings {pull}9948[9948]


This also needs a note, that the stat metricset was removed.

exekias added enhancement in progress Pull request is currently in progress. module discuss Issue needs further discussion. Metricbeat Metricbeat breaking change labels Jan 8, 2019

exekias requested review from a team as code owners January 8, 2019 12:50

houndci-bot reviewed Jan 8, 2019

View reviewed changes

exekias added the v7.0.0-beta1 label Jan 8, 2019

Carlos Pérez-Aradros Herce added 3 commits January 8, 2019 15:36

Update tests and data.json

e3004d0

Add missing comments

c2fc75e

foo

86bc56a

urso removed the request for review from a team January 8, 2019 17:31

jsoriano reviewed Jan 8, 2019

View reviewed changes

Correctly treat +Inf and quantiles encoding

411cc77

exekias force-pushed the refactor-prometheus branch from aaec74f to 411cc77 Compare January 9, 2019 09:54

fix test assertion after changes

7ac6a9b

exekias removed the discuss Issue needs further discussion. label Jan 14, 2019

Carlos Pérez-Aradros Herce added 2 commits January 18, 2019 16:52

Update changelog

28167f9

Update docs

ec1feed

exekias force-pushed the refactor-prometheus branch from ab7a29f to ec1feed Compare January 22, 2019 14:21

Merge branch 'master' into refactor-prometheus

229c108

exekias force-pushed the refactor-prometheus branch from 906f069 to 229c108 Compare January 22, 2019 14:49

ruflin reviewed Jan 22, 2019

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits January 23, 2019 13:51

Update docs

694d58d

Update docs to be more specific about what the module does

940734d

Carlos Pérez-Aradros Herce added 2 commits January 24, 2019 14:44

make update

5a970c4

Merge branch 'master' into refactor-prometheus

5657e00

ruflin approved these changes Jan 25, 2019

View reviewed changes

Carlos Pérez-Aradros Herce added 2 commits January 25, 2019 13:12

Update changelog

b6af126

Move Prometheus metrics under prometheus.metrics namespace

2536029

jsoriano approved these changes Jan 25, 2019

View reviewed changes

ruflin approved these changes Jan 25, 2019

View reviewed changes

exekias merged commit 70b3b35 into elastic:master Jan 25, 2019

This was referenced Jan 25, 2019

Enhancements of the Prometheus module #3456

Closed

Improve Prometheus integration documentation #9792

Closed

This was referenced Apr 2, 2019

Add _bucket to histogram metrics in Prometheus Collector #11578

Merged

Unable to fetch prometheus c++ exporter stats on metricbeat #8291

Closed

fearful-symmetry mentioned this pull request Apr 18, 2019

docs for Metricbeat Prometheus module show confusing port for scraping Prometheus exporters #11282

Closed

ismael-hasan mentioned this pull request Dec 10, 2019

[Metricbeat][docs] The namespace configuration seems to be mandatory in 6.8 in the Prometheus module, but it is commented out in the docs #15033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Prometheus metric mappings #9948

Refactor Prometheus metric mappings #9948

exekias commented Jan 8, 2019 •

edited

Loading

houndci-bot Jan 8, 2019

houndci-bot Jan 8, 2019

jsoriano left a comment

jsoriano Jan 8, 2019

jsoriano Jan 8, 2019

exekias Jan 9, 2019

jsoriano Jan 10, 2019

exekias Jan 10, 2019

jsoriano Jan 8, 2019

exekias commented Jan 9, 2019

ruflin commented Jan 9, 2019

exekias commented Jan 9, 2019

jsoriano commented Jan 10, 2019

exekias commented Jan 10, 2019

ruflin commented Jan 11, 2019

roncohen commented Jan 14, 2019

exekias commented Jan 14, 2019

exekias commented Jan 22, 2019

ruflin left a comment

ruflin Jan 22, 2019

ruflin Jan 22, 2019

ruflin Jan 22, 2019

ruflin Jan 22, 2019

exekias Jan 24, 2019

ruflin commented Jan 24, 2019

exekias commented Jan 25, 2019

ruflin left a comment

ruflin Jan 25, 2019

		@@ -69,6 +69,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d

		Metricbeat

		- Refactor Prometheus metric mappings {pull}9948[9948]

Refactor Prometheus metric mappings #9948

Refactor Prometheus metric mappings #9948

Conversation

exekias commented Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano 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

exekias commented Jan 9, 2019

ruflin commented Jan 9, 2019

exekias commented Jan 9, 2019

jsoriano commented Jan 10, 2019

exekias commented Jan 10, 2019

ruflin commented Jan 11, 2019

roncohen commented Jan 14, 2019

exekias commented Jan 14, 2019

exekias commented Jan 22, 2019

ruflin 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

ruflin commented Jan 24, 2019

exekias commented Jan 25, 2019

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Jan 8, 2019 •

edited

Loading