Add mapping for munin and options to override service type and name #10322

jsoriano · 2019-01-24T15:25:02Z

Make munin module more future-proof in its way to GA by adding a more strict
field mapping and name validation, and options to override ECS fields for
service type and name.

Changes here:

All munin metrics moved to munin.metrics, this way we can add more
munin subfields in the future if needed.
An event is generated per plugin now.
Everything in munin.metrics.* is expected to be a numeric metric.

jsoriano · 2019-01-24T15:27:46Z

jenkins, test this again please

ruflin · 2019-01-25T12:51:15Z

If we go with munin.metrics we would have it very similar to what we do with Prometheus. We could remove the namespace option. Instead we would introduce configuration of service.name and service.type. for filtering per service. Or even have namespace as a config option but it does not change the metric prefix.

Could it happen that 2 different services create a conflict?

@exekias Your input here would be great.

jsoriano · 2019-01-25T14:38:25Z

If we go with munin.metrics we would have it very similar to what we do with Prometheus.

Ok, I'll go for munin.metrics. I think that a good addition in the future could be to extract labels from the metric names, and split events by that. We could keep the metrics in munin.metrics and the extracted labels in munin.labels or in ECS fields. In any case this won't be breaking because metric names could follow the same rules.

We could remove the namespace option. Instead we would introduce configuration of service.name and service.type. for filtering per service. Or even have namespace as a config option but it does not change the metric prefix.

Do we have a name for the settings to override service.type and service.name?

I'll use namespace by now to set both values instead of adding it to the metric names.

Could it happen that 2 different services create a conflict?

Metric names should match ^[a-zA-Z_][a-zA-Z0-9_]*$, the dot has special meanings in munin, so I don't think we can have conflicts because of that. We are not checking now that the names are correct, I could add a check to avoid problems caused by wrong plugins.

There can be two plugins writing to the same fields things with different meanings, in that case there won't be mapping conflicts and the user can set namespace/service.type to differentiate.

exekias · 2019-01-25T15:34:44Z

metricbeat/module/munin/_meta/fields.yml

@@ -4,8 +4,10 @@
    Munin node metrics exporter
  release: beta
  fields:
-    - name: munin
-      type: group
+    - name: munin.*


you can remove the .* part, it seems that's how we normally do it in fields.yml, the timeseries.instance code will be easier too

This looks needed in this case, maybe because I am setting up dynamic templates for two levels now.

exekias · 2019-01-25T15:36:47Z

metricbeat/module/munin/_meta/fields.yml

+    - name: munin.*
+      type: object
+      object_type: double
+      object_type_mapping_type: 'long'


why long here? I would perhaps expect:
object_type_mapping_type: "*"

I needed objects to keep being objects, I have added two patterns now, one for the level of groups and another one for the metrics.

exekias · 2019-01-25T15:37:41Z

I don't have a strong opinion on prepending metrics or not, great to see this implemented here too!

metricbeat/module/munin/node/config.go

metricbeat/module/munin/_meta/fields.yml

metricbeat/module/munin/munin.go

ruflin · 2019-01-28T09:05:25Z

metricbeat/module/munin/node/node.go

-	return event, nil
-
+	event := mb.Event{
+		Service: m.serviceType,


The service type should not be munin by the service that is monitored. So if apache is monitored with munin, service.type should be apache. Because of this we should also have it set by the user. I wonder if we should make it required for the user to have it set or make it optional?

I had a discussion with @exekias and he proposed to have this as general option for all modules instead. Can you remove it from here and potentially open a separate PR with introducing these settings?

The service type should not be munin by the service that is monitored. So if apache is monitored with munin, service.type should be apache. Because of this we should also have it set by the user. I wonder if we should make it required for the user to have it set or make it optional?

After reading this I think that another option for events could be to use the plugin name as a different field and not for the path, so if we have now an event like this:

{ "service": { "type": "munin", "name": "somerole" }, { "munin": { "metrics": { "apache": { "accesses": 42, "errors": 2 }, "cpu": { "system": 0.8 }, } } }

Have instead two like this:

{ "service": { "type": "apache", "name": "somerole" }, { "observer": { "type": "munin" }, "munin": { "plugin": { "name": "apache" }, "metrics": { "accesses": 42, "errors": 2 } } }

{ "service": { "type": "cpu", "name": "somerole" }, { "observer": { "type": "munin" }, "munin": { "plugin": { "name": "cpu", }, "metrics": { "system": 0.8 } } }

This would be more aligned with ECS-like metrics.

I had a discussion with @exekias and he proposed to have this as general option for all modules instead. Can you remove it from here and potentially open a separate PR with introducing these settings?

I totally agree with having an option for service.name for all modules. Not so sure for service.type, I think that the type should be set in general by the module (do we want to allow arbitrary service.type for service modules?)

+1 on only allowing to set the service.type for the "input" modules. We should not allow to have it overwritten in other modules

It's very interesting that you set the observer above as munin and not metricbeat. @webmat This gets even more interesting now ;-)

I like the idea of the plugin field. How easily can this mapping be done? I assume we only have data from 1 plugin at the time?

I will add a general option for service.name in other PR.

The setting of service.type and observer.type will also go in another PR, I think there can be some shared code for that in all "input" modules.

The plugin field would be quite easy, the items we are looping on here, should rather be called plugins 🙂. I'll give a try to this here.

PR to add service.name #10427

ruflin

I'm pretty excited about this change. Especially the split up by plugin looks really good.

ruflin · 2019-01-30T10:36:59Z

metricbeat/metricbeat.reference.yml

@@ -507,7 +507,15 @@ metricbeat.modules:
  enabled: true
  period: 10s
  hosts: ["localhost:4949"]
-  node.namespace: node


Should we mention this removal in the changelog?

I removed this mention now that the refactor is so impactful. But I'll re-add something about that, yes 👍

Ensure all munin metrics are mapped to double

965ae7d

jsoriano added module discuss Issue needs further discussion. Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Jan 24, 2019

jsoriano self-assigned this Jan 24, 2019

jsoriano requested review from a team as code owners January 24, 2019 15:25

jsoriano mentioned this pull request Jan 24, 2019

[Metricbeat] Release munin as GA. #10311

Merged

exekias reviewed Jan 25, 2019

View reviewed changes

Put munin metrics under munin.metrics

3783213

jsoriano added the review label Jan 25, 2019

jsoriano added 2 commits January 25, 2019 18:36

Replace namespace option with service.type and service.name

68841a3

Merge remote-tracking branch 'origin/master' into munin-fields

85208b1

jsoriano changed the title ~~Add mapping for munin~~ Add mapping for munin and options to override service type and name Jan 25, 2019

Add missing file

81fc075

houndci-bot reviewed Jan 25, 2019

View reviewed changes

metricbeat/module/munin/node/config.go Show resolved Hide resolved

Make hound ci happier

32fed9c

ruflin reviewed Jan 28, 2019

View reviewed changes

jsoriano added 4 commits January 29, 2019 17:17

Generate an event per plugin, optionally sanitize field names

b0223c7

Merge remote-tracking branch 'origin/master' into munin-fields

0a29aa1

Fix changelog

72932fa

Fix typo

5427e78

ruflin approved these changes Jan 30, 2019

View reviewed changes

Add note about removal of namespace

aa1c230

jsoriano added 2 commits January 30, 2019 12:11

Merge remote-tracking branch 'origin/master' into munin-fields

d6d437f

Update x-pack metricbeat

b122bc9

jsoriano added the breaking change label Jan 30, 2019

ruflin approved these changes Jan 31, 2019

View reviewed changes

jsoriano merged commit 22f530a into elastic:master Jan 31, 2019

jsoriano deleted the munin-fields branch January 31, 2019 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mapping for munin and options to override service type and name #10322

Add mapping for munin and options to override service type and name #10322

jsoriano commented Jan 24, 2019 •

edited

Loading

jsoriano commented Jan 24, 2019

ruflin commented Jan 25, 2019

jsoriano commented Jan 25, 2019

exekias Jan 25, 2019

jsoriano Jan 25, 2019

exekias Jan 25, 2019

jsoriano Jan 25, 2019

exekias commented Jan 25, 2019

ruflin Jan 28, 2019

ruflin Jan 28, 2019 •

edited

Loading

jsoriano Jan 28, 2019 •

edited

Loading

jsoriano Jan 28, 2019 •

edited

Loading

ruflin Jan 29, 2019

jsoriano Jan 29, 2019 •

edited

Loading

jsoriano Jan 30, 2019

ruflin left a comment

ruflin Jan 30, 2019

jsoriano Jan 30, 2019

Add mapping for munin and options to override service type and name #10322

Add mapping for munin and options to override service type and name #10322

Conversation

jsoriano commented Jan 24, 2019 • edited Loading

jsoriano commented Jan 24, 2019

ruflin commented Jan 25, 2019

jsoriano commented Jan 25, 2019

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 25, 2019

Choose a reason for hiding this comment

ruflin Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jsoriano Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

jsoriano Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

jsoriano commented Jan 24, 2019 •

edited

Loading

ruflin Jan 28, 2019 •

edited

Loading

jsoriano Jan 28, 2019 •

edited

Loading

jsoriano Jan 28, 2019 •

edited

Loading

jsoriano Jan 29, 2019 •

edited

Loading