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

Use textparse in prometheus module helper library #33865

Merged
merged 22 commits into from Jan 25, 2023

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Nov 29, 2022

What does this PR do?

Switches to using textparse as discussed at #24707.

This PR extends #27269 so as to make use the textparse in the prometheus helper library which is used by the prometheus, kubernetes and containerd modules.

This will solve the issue of handling the duplicated metrics reported at #18813 -> Testcase

cc: @jsoriano @premendrasingh @vjsamuel

Closes #18813

Testing TODOs

In all cases the number of metrics and their types should be the same with the old and and new library.

Manually test the kubernetes module to ensure that state-* metricsets are not broken

Verified number of events in both branches.

Manually test the prometheus module against a node-exporter

Tested against prom/node-exporter:v0.18.1 . This branch and main collect the same number of metrics and labels. This ensure us that we collect and store the same number of fields.
nodeExporterLabels
nodeExporterMetrics

Manually test the prometheus module against a node-exporter using types and rates

This branch and main collect the same number of metrics and labels. This ensure us that we collect and store the same number of fields.

For the rest of the sanity checks we can rely on unit tests, since those evaluate the produced values too.

node-exporter appendix
  1. Define docker-compose.yml as:
version: '2.1'

services:
  nodeexporter:
    command:
    - --path.procfs=/host/proc
    - --path.rootfs=/rootfs
    - --path.sysfs=/host/sys
    - --collector.filesystem.ignored-mount-points=^/(sys|proc|dev|host|etc)($$|/)
    image: prom/node-exporter:v0.18.1
    expose:
    - 9100
    ports:
    - 9100:9100
    restart: unless-stopped
    volumes:
    - /proc:/host/proc:ro
    - /sys:/host/sys:ro
    - /:/rootfs:ro
  1. Deploy node-exporter with docker-compose up
  2. The prometheus endpoint should be available at 0.0.0.0:9100
  3. Enable the prometheus module of metricbeat and configure the collector metricset to collect from 0.0.0.0:9100
  4. Start Metricbeat and see metrics flowing to ES

Histogram checks

In order to test that histograms are calculated properly use the following complete example of Prometheus

version: '2.1'
volumes:
  prometheus_data: {}

services:

  prometheus:
    command:
    - --config.file=/etc/prometheus/prometheus.yml
    - --storage.tsdb.path=/prometheus
    - --web.console.libraries=/etc/prometheus/console_libraries
    - --web.console.templates=/etc/prometheus/consoles
    - --storage.tsdb.retention.time=200h
    - --web.enable-lifecycle
    expose:
    - 9090
    image: prom/prometheus:v2.16.0
    ports:
    - 9090:9090
    restart: unless-stopped
    volumes:
    - ./prometheus:/etc/prometheus
    - prometheus_data:/prometheus
   
  nodeexporter:
    command:
    - --path.procfs=/host/proc
    - --path.rootfs=/rootfs
    - --path.sysfs=/host/sys
    - --collector.filesystem.ignored-mount-points=^/(sys|proc|dev|host|etc)($$|/)
    image: prom/node-exporter:v0.18.1
    expose:
    - 9100
    ports:
    - 9100:9100
    restart: unless-stopped
    volumes:
    - /proc:/host/proc:ro
    - /sys:/host/sys:ro
    - /:/rootfs:ro

and

global:
  evaluation_interval: 5s
  scrape_interval: 5s

scrape_configs:
- job_name: nodeexporter
  scrape_interval: 5s
  static_configs:
  - targets:
    - nodeexporter:9100

Histograms can be found at prometheus-ip:9090/metrics.

@ChrsMark ChrsMark self-assigned this Nov 29, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2022

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @ChrsMark? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 29, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-24T11:04:16.816+0000

  • Duration: 74 min 39 sec

Test stats 🧪

Test Results
Failed 0
Passed 4584
Skipped 1025
Total 5609

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added the Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team label Dec 13, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 13, 2022
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark marked this pull request as ready for review December 14, 2022 15:08
@ChrsMark ChrsMark requested review from a team as code owners December 14, 2022 15:08
@ChrsMark ChrsMark requested review from fearful-symmetry and leehinman and removed request for a team December 14, 2022 15:08
@ChrsMark
Copy link
Member Author

Opening for an early review since we agreed on moving this one forward. Tests have been added, and unit tests of both prometheus and kubernetes module are not breaking. This is a good indicator by now that we don't break the already existent functionalities. I will also focus on testing these 2 scenarios manually too but any review at this point more than welcome.

@ChrsMark ChrsMark changed the title [WiP] Use textparse in prometheus module helper library Use textparse in prometheus module helper library Dec 15, 2022
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.

Most of my comments are on code moved in the textparse.go files, so feel free to ignore them by now as this is not added on this PR.

metricbeat/helper/openmetrics/metric.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/prometheus.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/prometheus.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/textparse.go Show resolved Hide resolved
metricbeat/helper/prometheus/textparse.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/textparse.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/prometheus.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/textparse.go Outdated Show resolved Hide resolved
metricbeat/helper/prometheus/textparse.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/prometheus/remote_write/data.go Outdated Show resolved Hide resolved
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@@ -192,19 +190,6 @@ func (g remoteWriteTypedGenerator) GenerateEvents(metrics model.Samples) map[str
return eventList
}

// rateCounterUint64 fills a counter value and optionally adds the rate if rate_counters is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not used at all and linter complains about this :)

@gizas
Copy link
Contributor

gizas commented Dec 16, 2022

Small one, can you add details how you installed and tested with prom/node-exporter:v0.18.1 ? Maybe an extra task to update our infraobs documentation?

Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
Signed-off-by: ChrsMark <chrismarkou92@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Dec 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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.

Most of my concerns have been addressed, LGTM, will leave the final decision to your team 🙂

Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

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

LGTM!

@ChrsMark
Copy link
Member Author

Since manual testing have been completed and unit tests are green I'm gonna merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7-candidate Team:Cloudnative-Monitoring Label for the Cloud Native Monitoring team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle duplicated TYPE line for prometheus metrics
5 participants