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.

Sign up for GitHub

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

Jump to bottom

Kubernetes state_daemonset metricset for Metricbeat #20649

Merged
merged 4 commits into from
Sep 1, 2020
Merged

Kubernetes state_daemonset metricset for Metricbeat #20649

merged 4 commits into from
Sep 1, 2020

Conversation

ninaspitfire
Copy link
Contributor

@ninaspitfire ninaspitfire commented Aug 18, 2020 •
edited
Loading

What does this PR do?

Implements state_daemonset for the Metricbeat Kubernetes module, in the model of state_deployment, state_replicaset etc.

Why is it important?

Allows monitoring (and alerting) of DaemonSet replica status. Gives DaemonSet similar Metricbeat coverage to the other standard pod controllers (ReplicaSet etc).

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

  • Generated documentation is sufficient and correct.

How to test this PR locally

MODULE=kubernetes mage -v test in the metricbeat directory.

Test result may be affected by #20627

Related issues

Use cases

Comprehensive alerting for all standard pod controllers. Alert when insufficient available pods exist for a DaemonSet, (not just Deployment, ReplicaSet, and StatefulSet).

@ninaspitfire
Kubernetes state_daemonset metricset for Metricbeat
d511981 
Implements state_daemonset for the Metricbeat Kubernetes module, in
the model of state_deployment, state_replicaset etc.

Closes: elastic#17010
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 18, 2020
@elasticmachine

This comment has been minimized.

Sign in to view
@andresrc andresrc added the Team:Platforms Label for the Integrations - Platforms team label Aug 18, 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 Aug 18, 2020
@ninaspitfire
Copy link
Contributor Author

Any interest in this patch? Is there anything you'd like to see changed?

Thanks.

ChrsMark
ChrsMark reviewed Sep 1, 2020
View reviewed changes
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.

In general looks good to me. Thank you for contributing this @jarpy !

@jsoriano what do you think?

metricbeat/module/kubernetes/state_daemonset/state_daemonset.go
DefaultPath: defaultPath,
}.Build()

mapping = &p.MetricsMapping{
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would make these struct fields instead of const variables. In this, they will be initialised when New is called and not at load time of the package.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the suggestion, but we are defining these mappings as vars in other metricsets too. So I think we can go on with this change and refactor these metricsets in a future PR.

jsoriano
jsoriano approved these changes Sep 1, 2020
View reviewed changes
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, thanks @jarpy, and sorry for the delay reviewing!

metricbeat/module/kubernetes/state_daemonset/state_daemonset.go
DefaultPath: defaultPath,
}.Build()

mapping = &p.MetricsMapping{
Copy link
Member

Choose a reason for hiding this comment

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

I agree with the suggestion, but we are defining these mappings as vars in other metricsets too. So I think we can go on with this change and refactor these metricsets in a future PR.

@jsoriano jsoriano added needs_backport PR is waiting to be backported to other branches. v7.10.0 labels Sep 1, 2020
@jsoriano jsoriano self-assigned this Sep 1, 2020
@jsoriano jsoriano merged commit 2a0e099 into elastic:master Sep 1, 2020
@jsoriano
Copy link
Member

jsoriano commented Sep 1, 2020

Oops, it was marked as experimental, I think we should make it at least beta, I will open another PR for this.

@jsoriano jsoriano mentioned this pull request Sep 1, 2020
Merged
7 tasks
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Sep 1, 2020
jsoriano pushed a commit to jsoriano/beats that referenced this pull request Sep 1, 2020
@ninaspitfire @jsoriano
Kubernetes state_daemonset metricset for Metricbeat (elastic#20649)
0159bac 
Implements state_daemonset for the Metricbeat Kubernetes module, in
the model of state_deployment, state_replicaset etc.

(cherry picked from commit 2a0e099)
@ninaspitfire
Copy link
Contributor Author

Thanks, folks! I'm so excited to have this in, both for personal and operational reasons.

Oops, it was marked as experimental

Sorry about that. I was trying to be as conservative as possible, because I'm a visitor in this house. :)

@jsoriano
Copy link
Member

jsoriano commented Sep 2, 2020

Sorry about that. I was trying to be as conservative as possible, because I'm a visitor in this house. :)

No worries, I should have seen it 🙂 In any case it has helped us to find that we had more metricsets marked as experimental #20901

v1v added a commit to v1v/beats that referenced this pull request Sep 2, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/ci-pipeli…
1faa11d 
…ne-2.0

* upstream/master: (87 commits)
  [packaging] Normalise GCP bucket folder structure (elastic#20903)
  [Metricbeat] Add billing metricset into googlecloud module (elastic#20812)
  Include python docs in devguide index (elastic#20917)
  Avoid generating incomplete configurations in autodiscover (elastic#20898)
  Improve docs of leaderelection configuration (elastic#20601)
  Document how to set the ES host and Kibana URLs in Ingest Manager (elastic#20874)
  docs: Update beats for APM (elastic#20881)
  Adding cborbeat to community beats (elastic#20884)
  Bump kibana version to 7.9.0 in x-pack/metricbeat (elastic#20899)
  Kubernetes state_daemonset metricset for Metricbeat (elastic#20649)
  [Filebeat][zeek] Add new x509 fields to zeek (elastic#20867)
  [Filebeat][Gsuite] Add note about admin in gsuite docs (elastic#20855)
  Ensure kind cluster has RFC1123 compliant name (elastic#20627)
  Setup python paths in test runner configuration (elastic#20832)
  docs: Add  `processor.event` info to Logstash output (elastic#20721)
  docs: update cipher suites (elastic#20697)
  [ECS] Update ecs to 1.6.0 (elastic#20792)
  Fix path in hits docs (elastic#20447)
  Update filebeat azure module documentation (elastic#20815)
  Remove duplicate ListGroupsForUsers in aws/cloudtrail (elastic#20788)
  ...
jsoriano added a commit that referenced this pull request Sep 3, 2020
@jsoriano @ninaspitfire
Cherry-pick #20649 to 7.x: Kubernetes state_daemonset metricset for M…
4afcb74 
…etricbeat (#20900)

Implements state_daemonset for the Metricbeat Kubernetes module, in
the model of state_deployment, state_replicaset etc.

(cherry picked from commit 2a0e099)

Co-authored-by: Toby McLaughlin <toby@jarpy.net>
melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020
@ninaspitfire @melchiormoulin
Kubernetes state_daemonset metricset for Metricbeat (elastic#20649)
a28d741 
Implements state_daemonset for the Metricbeat Kubernetes module, in
the model of state_deployment, state_replicaset etc.
@ChrsMark ChrsMark mentioned this pull request Jan 26, 2021
Merged
2 tasks
@ninaspitfire ninaspitfire deleted the state-daemonset branch February 10, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrsMark ChrsMark ChrsMark approved these changes

@jsoriano jsoriano jsoriano approved these changes

Assignees

@jsoriano jsoriano

Labels
enhancement Team:Platforms Label for the Integrations - Platforms team v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add state_daemonset metricset to Kubernetes module
5 participants
@ninaspitfire @elasticmachine @jsoriano @ChrsMark @andresrc