Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OpenMetrics Metricbeat module #16596

Merged
merged 16 commits into from
Mar 4, 2020
Merged

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Feb 26, 2020

What does this PR do?

This PR adds OpenMetrics Metricbeat module as a light module on top of Prometheus collector metricset.

Why is it important?

OpenMetrics is an effort to standardize exposing metric data based on the Prometheus exposition format. This standalone module will provide an easy and out of the box way to collect metrics from endpoints that follow this format.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

  1. One can use a nodexporter as an example Prometheus app to collect from. For instance this forked project should be enough: https://github.com/ChrsMark/dockprom. Download and docker-compose up. Then metrics should be exported at http://localhost:9100/metrics.
  2. Configuration:
- module: openmetrics
  metricsets: ['collector']
  period: 10s
  hosts: ['localhost:9100']

  # This module uses the Prometheus collector metricset, all
  # the options for this metricset are also available here.
  metrics_path: /metrics
  metrics_filters:
    include: ["node_filesystem_*"]
    exclude: ["node_filesystem_device_*", "^node_filesystem_readonly$"]

And expect to see only events that match node_filesystem_* but also not node_filesystem_device_* and are not node_filesystem_readonly.
3. Try to remove ^node_filesystem_readonly$ from exclude filters and see that this metric is included in events. One can try different combinations.

Related issues

@ChrsMark ChrsMark added enhancement module review needs_backport PR is waiting to be backported to other branches. containers Related to containers use case [zube]: In Review Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan v7.7.0 Team:Platforms Label for the Integrations - Platforms team labels Feb 26, 2020
@ChrsMark ChrsMark requested a review from a team February 26, 2020 10:36
@ChrsMark ChrsMark self-assigned this Feb 26, 2020
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some thoughts, but some things are going to depend on how OpenMetrics is released at the end. Let me know what you think.

In any case I would be ok with merging this by now as an initial implementation.

"name": "collector",
"period": 10000
},
"prometheus": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ok with using prometheus in openmetrics fields?

In my opinion if we would be building prometheus/openmentrics support from scratch it could make sense to use openmetrics terminology, but given that we are already using prometheus, and most current implementations are prometheus-based, I am ok with using prometheus also for open metrics field names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no strong opinion on this. Actually it was sth I had as an open question too. Maybe @exekias or @sorantis could help with this decision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on using openmetrics namespace, specially since we expect the protocol & output to change a bit

x-pack/metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/metricbeat/module/openmetrics/module.yml Outdated Show resolved Hide resolved
@ChrsMark
Copy link
Member Author

Tried to make of Namespace functionality that we already have so as to put metrics under openmetrics namespace. One implication is that this is gonna change the way we write event.dataset fields for prometheus bases modules because of this

"dataset": event.Namespace,

, here is a diff:

     "event": {
-        "dataset": "prometheus.collector",
+        "dataset": "prometheus",
         "duration": 115000,
         "module": "prometheus"
     },

@jsoriano @exekias would this be acceptable? I think this only affects prometheus, cochroachdb and ibmmq modules. wdyt?

@jsoriano
Copy link
Member

@jsoriano @exekias would this be acceptable? I think this only affects prometheus, cochroachdb and ibmmq modules. wdyt?

I guess we should use prometheus.collector as namespace then? Or maybe we should review how the namespace affects to the value of event.dataset.

@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 27, 2020

@jsoriano @exekias would this be acceptable? I think this only affects prometheus, cochroachdb and ibmmq modules. wdyt?

I guess we should use prometheus.collector as namespace then? Or maybe we should review how the namespace affects to the value of event.dataset.

If we use prometheus.collector as namespace then we break the schema, since metrics would go under prometheus.collector.*.

@ChrsMark
Copy link
Member Author

I see that this field was introduced at #8941.

In my view it is quite robust. We report prometheus metrics under prometheus.* so dataset makes sense to be prometheus too and not prometheus.collector and the same goes for light modules that are build on top of it since they report metrics under the same namespace.

So I would consider it safe to change it since there are still event.module and metricset.name which can be also used.

@@ -1,7 +1,7 @@
[
{
"event": {
"dataset": "ibmmq.qmgr",
"dataset": "prometheus",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, I don't like this change, I didn't fully get the implications of the namespace changes in prometheus collector.

It was intentional to have the name of the metricset in event.dataset. This way we have all the prometheus metrics under prometheus.metrics, but we can still use event.dataset to know the source of the metrics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would consider it safe to change it since there are still event.module and metricset.name which can be also used.

Umm, this could be a breaking change if something or someone is using event.dataset for this purpouse (I would have used this field myself, and I have probably recommended it in the past).

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 27, 2020

hmm, I got your point @jsoriano. A workaround could be what I tried and implemented, to pass the "namespace" to the factory method and use within the metricset accordingly. Let me know what you think.

ps: sorry for the force-push but I had to drop the commit that changed golden files.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, "too many open files" errors in CI are fixed in master.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 4, 2020

Merging this, since Libbeat test's(https://travis-ci.org/elastic/beats/jobs/658099652?utm_medium=notification&utm_source=github_status) are also failing on master.

@ChrsMark ChrsMark merged commit 3b35d0b into elastic:master Mar 4, 2020
@zube zube bot added [zube]: Done and removed [zube]: Inbox labels Mar 4, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Mar 4, 2020
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Mar 4, 2020
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case enhancement module review Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants