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 support for multiple sets of hints on autodiscover #18883

Merged
merged 3 commits into from
Jun 23, 2020

Conversation

vjsamuel
Copy link
Contributor

@vjsamuel vjsamuel commented Jun 1, 2020

Enhancement

What does this PR do?

This PR allows defining multiple sets of annotations similar to how processors are defined. This functionality already exists in heartbeat. This PR standardizes the utility and adds it to logs and metrics hints builder as well.

Why is it important?

A container can always have more than one kind of endpoint that requires different kinds of processing needs. We need to provide a uniform experience across all beats from a hints perspective.

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.

Author's Checklist

  • [ ]

How to test this PR locally

  • Check for regressions:
    • Use heartbeat with hints enabled and setup a pod with multiple monitors, this should continue working
    • Use metricbeat or filebeat with hints enabled and setup a pod with hints as usually configured, this should continue working.
    • Repeat the previous test with container-specific hints.
  • Test the new feature:
    • Use metricbeat or filebeat with hints enabled, and setup a pod with multiple sets of hints (there are some examples in the docs added in this PR), check that multiple modules are instantiated for the pod.
    • Repeat the previous test with container-specific hints.

Related issues

Use cases

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jun 1, 2020
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@vjsamuel vjsamuel force-pushed the add_multiple_hints_support branch from 8d6bfb0 to b4c57d9 Compare June 1, 2020 17:36
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 1, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #18883 updated]

  • Start Time: 2020-06-22T06:43:43.409+0000

  • Duration: 80 min 16 sec

Test stats 🧪

Test Results
Failed 0
Passed 9481
Skipped 1574
Total 11055

@andresrc andresrc added the Team:Platforms Label for the Integrations - Platforms team label Jun 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jun 2, 2020
@exekias
Copy link
Contributor

exekias commented Jun 4, 2020

@ChrsMark @jsoriano what do you think about this one? @vjsamuel could you please explain a little bit about the use case that you are trying to solve?

@vjsamuel
Copy link
Contributor Author

vjsamuel commented Jun 4, 2020

this allows use cases like:

co.elastic.metrics/1.module: prometheus
co.elastic.metrics/1.hosts: ${data.host}:8080/metrics
co.elastic.metrics/1.period: 1m

co.elastic.metrics/2.module: prometheus
co.elastic.metrics/2.hosts: ${data.host}:8080/app/metrics
co.elastic.metrics/2.period: 10s

this is currently not doable.

@jsoriano
Copy link
Member

jsoriano commented Jun 5, 2020

I think this feature makes sense, also thinking on cases like kafka, where we may want to do something like this:

co.elastic.metrics/1.module: kafka
co.elastic.metrics/1.metricsets: consumergroup,partition
co.elastic.metrics/1.hosts: ${data.host}:9092

co.elastic.metrics/2.module: kafka
co.elastic.metrics/2.metricsets: broker
co.elastic.metrics/2.hosts: ${data.host}:8778

So this enhancement has my +1, but I also think that it increases the complexity and we have to be very careful.

We have to be clear on how this feature would work, for example, would a mix of numbered and unnumbered configurations work? and if it is, what configs would it produce?
Something like the following would be allowed? would it create two configs or only one? what would be the period?

co.elastic.metrics/module: prometheus
co.elastic.metrics/hosts: ${data.host}:8080/metrics
co.elastic.metrics/period: 1m
co.elastic.metrics/1.module: prometheus
co.elastic.metrics/1.hosts: ${data.host}:8080/app/metrics

We also need to be clear on how we are going to document this. Autodiscover is already quite powerful, but also sometimes difficult to understand by some users. Currently we document the possible annotations with its whole name (e.g: co.elastic.metrics/module), we are going to think very well how to add the documentation for this feature.

And we are also going to need thorough testing, this feature opens the gate to many kinds of cases.

@vjsamuel vjsamuel force-pushed the add_multiple_hints_support branch 3 times, most recently from a0eed91 to 2e431b8 Compare June 15, 2020 07:48
@vjsamuel vjsamuel requested a review from a team as a code owner June 15, 2020 07:48
@vjsamuel vjsamuel force-pushed the add_multiple_hints_support branch 3 times, most recently from f18fa4c to 09e06c8 Compare June 16, 2020 19:12
@jsoriano jsoriano self-assigned this Jun 17, 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.

@vjsamuel thanks for remarking that this is a feature already available in heartbeat, I see this as an strong point to make it available as is in other beats 🙂 And thanks for improving the documentation!

I have been taking a quick look to the PR that introduced hints in Heartbeat (#13119), and there you mentioned that Heartbeat seems to be a unique use case as a user might want more than one check on his/her pod/container. Out of curiosity, what made you change your opinion? Is there a use case worth mentioning for other beats?

I am going to add this PR to the manual test plan so we will make additional manual testing before releasing it, I have added some notes for it in the how to test section in the issue description, please review it and feel free to modify it.

filebeat/docs/autodiscover-hints.asciidoc Show resolved Hide resolved
@jsoriano jsoriano added needs_backport PR is waiting to be backported to other branches. test-plan Add this PR to be manual test plan v7.9.0 labels Jun 17, 2020
@vjsamuel
Copy link
Contributor Author

What helped change my mind? One of the reasons is that the prometheus endpoint having the jvm/go runtime metrics causes a stop the world pause to get the memory snapshot. for latency sensitive applications we prefer not to scrape these endpoints at 5s granularity. However, we do have application metrics that need such scrape intervals. so, the natural tendency is to push those metrics to a different registry and scrape them at a lower granularity as compared to the runtime metrics.

I had no good reason to do it for filebeat but its a great feature :)

@vjsamuel vjsamuel force-pushed the add_multiple_hints_support branch 2 times, most recently from ddcf32d to 1c54e3f Compare June 18, 2020 16:04
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, improvements in docs and tests cover my original concerns, thanks @vjsamuel!

@jsoriano
Copy link
Member

jenkins, run the tests please

@jsoriano jsoriano merged commit a5da85c into elastic:master Jun 23, 2020
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Jun 23, 2020
Allows defining multiple sets of annotations similar to how processors are defined.
This functionality already exists in heartbeat. This change standardizes the utility
and adds it to logs and metrics hints builder as well.

(cherry picked from commit a5da85c)
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Jun 23, 2020
jsoriano added a commit that referenced this pull request Jun 23, 2020
Allows defining multiple sets of annotations similar to how processors are defined.
This functionality already exists in heartbeat. This change standardizes the utility
and adds it to logs and metrics hints builder as well.

(cherry picked from commit a5da85c)

Co-authored-by: Vijay Samuel <[email protected]>
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Jul 14, 2020
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
Allows defining multiple sets of annotations similar to how processors are defined.
This functionality already exists in heartbeat. This change standardizes the utility
and adds it to logs and metrics hints builder as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants