Skip to content

[metricbeat] [system] add NTP metricset #44884

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

Merged
merged 20 commits into from
Jul 18, 2025

Conversation

tommyers-elastic
Copy link
Contributor

@tommyers-elastic tommyers-elastic commented Jun 17, 2025

Proposed commit message

Add new Network Time Protocol (NTP) metricset to the system integration.

This is a simple wrapper around an SNTP client which allows monitoring of system clock offset compared to the configured NTP server.

Screenshot 2025-06-17 at 17 14 11

Example test configuration:

metricbeat.modules:
  - module: system
    metricsets: 
     - ntp
    ntp:
      servers: ["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org", "3.pool.ntp.org"]
      timeout: 2s
      version: 4

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.

@tommyers-elastic tommyers-elastic added the Team:Integrations Label for the Integrations team label Jun 17, 2025
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 17, 2025
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

Copy link
Contributor

mergify bot commented Jun 17, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @tommyers-elastic? 🙏.
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-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

Copy link
Contributor

@mykola-elastic mykola-elastic left a comment

Choose a reason for hiding this comment

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

Even though it is in "draft" and the checks are not yet fixed, it looks quite good to me

Copy link
Contributor

mergify bot commented Jun 24, 2025

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 system/ntp upstream/system/ntp
git merge upstream/main
git push upstream system/ntp

@tommyers-elastic tommyers-elastic marked this pull request as ready for review July 9, 2025 16:04
@tommyers-elastic tommyers-elastic requested review from a team as code owners July 9, 2025 16:04
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jul 9, 2025
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

Copy link
Contributor

github-actions bot commented Jul 10, 2025

Copy link
Contributor

@stefans-elastic stefans-elastic left a comment

Choose a reason for hiding this comment

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

lgtm

@tommyers-elastic tommyers-elastic enabled auto-merge (squash) July 11, 2025 09:54
Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

I'm trying to test it but it does not work for me.
As you can see in my comments, it seems to me you're not setting the host correctly.

this is how I tested:

  • build metricbeat: go build .
  • run it go run . --strict.perms=false -e -c ./metricbeat-anderson.yml
  • my confg:
path.home: /tmp/beats/home
metricbeat.modules:
  - module: system
    period: 1s
    metricsets:
      - ntp
    ntp.hosts: ["localhost", "pool.ntp.org"]

output.file:
  path: /tmp/beats/home
  filename: "output-metricbeat"

logging.level: debug
logging.metrics:
  level: debug

What I'm gerring:

{"log.level":"error","@timestamp":"2025-07-11T16:17:30.550+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2025-07-11T16:17:31.551+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2025-07-11T16:17:32.551+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2025-07-11T16:17:33.551+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2025-07-11T16:17:34.550+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2025-07-11T16:17:35.551+0200","log.logger":"system.ntp","log.origin":{"function":"github.com/elastic/beats/v7/metricbeat/mb/module.(*metricSetWrapper).handleFetchError","file.name":"module/wrapper.go","file.line":335},"message":"Error fetching data for metricset system.ntp: error querying NTP server : address string is empty","service.name":"metricbeat","ecs.version":"1.6.0"}

Could you please add a the section "How to test this PR locally" to facilitate others testing it?

Copy link
Contributor

mergify bot commented Jul 11, 2025

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 system/ntp upstream/system/ntp
git merge upstream/main
git push upstream system/ntp

@tommyers-elastic
Copy link
Contributor Author

tommyers-elastic commented Jul 14, 2025

@AndersonQ the hosts config should not be prefixed

metricbeat.modules:
  - module: system
    period: 1s
    metricsets: ["ntp"]
    hosts: ["0.pool.ntp.org", "1.pool.ntp.org"]

i'll update this - because i see now that if you configured multiple metricsets here it errors out (see output below if you also configure cpu). the NTP config needs to be namespaced.

Exiting: host parsing failed for system-cpu: hosts must be empty for system

@AndersonQ
Copy link
Member

@AndersonQ the hosts config should not be prefixed

metricbeat.modules:
  - module: system
    period: 1s
    metricsets: ["ntp"]
    hosts: ["0.pool.ntp.org", "1.pool.ntp.org"]

i'll update this - because i see now that if you configured multiple metricsets here it errors out (see output below if you also configure cpu). the NTP config needs to be namespaced.

Exiting: host parsing failed for system-cpu: hosts must be empty for system

ok, let me know when it's ready for re-review

@tommyers-elastic
Copy link
Contributor Author

@AndersonQ we should be good to go

Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

looks good now. If all tests pass, it's good

Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Some small suggestions (and big, but not blocking, thoughts) below.

tommyers-elastic and others added 3 commits July 16, 2025 23:10
Co-authored-by: Colleen McGinnis <colleen.j.mcginnis@gmail.com>
Co-authored-by: Colleen McGinnis <colleen.j.mcginnis@gmail.com>
Co-authored-by: Colleen McGinnis <colleen.j.mcginnis@gmail.com>
Copy link
Contributor

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

Definition lists look good now!

image

Copy link
Contributor

@lalit-satapathy lalit-satapathy left a comment

Choose a reason for hiding this comment

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

Code owner approvals.

@tommyers-elastic tommyers-elastic merged commit 5a6779f into elastic:main Jul 18, 2025
205 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.