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

Openmetrics support using textparser #27269

Merged
merged 77 commits into from
Nov 17, 2021

Conversation

premendrasingh
Copy link
Contributor

@premendrasingh premendrasingh commented Aug 6, 2021

Enhancement - Added openmetrics module to use Openmetrics parser.

What does this PR do?

This PR updated metricset in Openmetrics module. It handles metrics endpoints returning data with
Content-Type: application/openmetrics-text

Why is it important?

Many of our customers have data exposed in OpenMetrics format, but existing module piggy backs on Prometheus module which processes metrics only in expfmt, Openmetrics data is not handled properly. Hence the need for Openmetrics support also.

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
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Unit tests in openmetrics/collector can be used to test the metrics locally.

Setup a http server to serve Openmetrics data with HTTP header Content-Type: application/openmetrics-text

Change metricset in metricbeat.yml to use openmetrics/collector as shown below

metricbeat.modules:
- module: openmetrics
  hosts: ["http://localhost:8080/metrics"]
  metricsets: ["collector"]

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 6, 2021
@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b openmetrics-collector upstream/openmetrics-collector
git merge upstream/master
git push upstream openmetrics-collector

@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 6, 2021

💚 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: 2021-11-16T10:34:06.284+0000

  • Duration: 47 min 41 sec

  • Commit: ff107b4

Test stats 🧪

Test Results
Failed 0
Passed 708
Skipped 239
Total 947

💚 Flaky test report

Tests succeeded.

🤖 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!)

@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b openmetrics-collector upstream/openmetrics-collector
git merge upstream/master
git push upstream openmetrics-collector

@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b openmetrics-collector upstream/openmetrics-collector
git merge upstream/master
git push upstream openmetrics-collector

@mergify
Copy link
Contributor

mergify bot commented Aug 13, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b openmetrics-collector upstream/openmetrics-collector
git merge upstream/master
git push upstream openmetrics-collector

@jsoriano jsoriano added the Team:Integrations Label for the Integrations team label Aug 19, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 19, 2021
@jsoriano
Copy link
Member

/test

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Thank for this PR @premendrasingh , it is going into the right direction I think!

I left some comments inline, mostly about minors, but overall the patch looks good. My two main concenrs are:

  1. I see a lot of code duplication between between this module and prometheus module around helper functionalities. Wondering if we could handle it better by introducing one common library?
  2. I see we add new types with PR. Maybe this needs evaluation product-wise.

@exekias @jsoriano your feedback is more than welcome here :).

metricbeat/docs/modules/linux/memory.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/collector_test.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/data.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/data.go Outdated Show resolved Hide resolved
return m.skipFamilyName(*family.Name)
}

func (m *MetricSet) skipFamilyName(family string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

I see/expect some replication of code between the 2 modules (prometheus and openmetircs) so maybe we can move those methods to a utility library when we refactor prometheus module too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will be better to have a separate PR for refactoring this into a utility library.

Copy link
Member

Choose a reason for hiding this comment

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

Yeap, doing this when we change prometheus module to use textparser or in a different follow-up sounds good.

metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
)

const (
openMetricsTestSamples = `# TYPE first_metric gauge
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add a case with exemplars?

Copy link
Contributor Author

@premendrasingh premendrasingh left a comment

Choose a reason for hiding this comment

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

Thanks for the review comments.

metricbeat/docs/modules/linux/memory.asciidoc Outdated Show resolved Hide resolved
- name: type
type: keyword
description: >
metric type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to Metric

metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
if m.skipFamily(family) {
continue
}
promEvents := m.promEventsGen.GenerateOpenMetricsEvents(family)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Changed to openMetricsEvents

eventList[promEvent.Type][labelsHash]["exemplar"] = promEvent.Exemplars
}
// Accumulate metrics in the event
eventList[promEvent.Type][labelsHash].DeepUpdate(promEvent.Data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

metricbeat/module/openmetrics/collector/collector_test.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/data.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/data.go Outdated Show resolved Hide resolved
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 haven't looked into the details of how each metric type is handled, but overall this is looking good.

go.mod Outdated
)

replace (
github.com/Azure/go-autorest => github.com/Azure/go-autorest v12.2.0+incompatible
github.com/Azure/go-autorest => github.com/Azure/go-autorest v14.2.0+incompatible
github.com/Microsoft/go-winio => github.com/bi-zone/go-winio v0.4.15
github.com/Shopify/sarama => github.com/elastic/sarama v1.19.1-0.20210120173147-5c8cb347d877
github.com/cucumber/godog => github.com/cucumber/godog v0.8.1
Copy link
Member

Choose a reason for hiding this comment

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

Many changes here seem unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream rebase removed it.

Copy link
Member

Choose a reason for hiding this comment

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

Umnm, there are still many other changes in go.sum that look unrelated (for google could, oracle, azure...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up go.mod, go.sum. Please guide me if it is still not right.

metricbeat/helper/openmetrics/openmetrics.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/mb/testing/testdata.go Outdated Show resolved Hide resolved
metricbeat/module/openmetrics/collector/collector_test.go Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b openmetrics-collector upstream/openmetrics-collector
git merge upstream/master
git push upstream openmetrics-collector

@jsoriano
Copy link
Member

jsoriano commented Sep 2, 2021

/test

@ChrsMark
Copy link
Member

ChrsMark commented Nov 3, 2021

@newly12 sorry for the delay here but upgrading the library got stuck a little bit but now it is merged (#28716) and we can move this one forward 🙂 .

@ChrsMark
Copy link
Member

/test

@ChrsMark
Copy link
Member

/test

@jsoriano jsoriano removed their request for review November 16, 2021 11:08
@jsoriano jsoriano dismissed their stale review November 16, 2021 11:12

Dismissing my old review.

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for contributing this one!

@ChrsMark ChrsMark added v8.1.0 enhancement and removed v7.16.0 backport-v7.16.0 Automated backport with mergify Team:Automation Label for the Observability productivity team labels Nov 17, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 17, 2021

This pull request does not have a backport label. Could you fix it @premendrasingh? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 17, 2021
@ChrsMark ChrsMark merged commit ddf8b47 into elastic:master Nov 17, 2021
@premendrasingh
Copy link
Contributor Author

Thanks a lot @ChrsMark

@jsoriano
Copy link
Member

Happy to see this merged 🙂 Thanks @premendrasingh and @ChrsMark!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants