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

timex: this plugin enables timex plugin for non-linux systems #12489

Merged
merged 5 commits into from
Mar 24, 2022
Merged

timex: this plugin enables timex plugin for non-linux systems #12489

merged 5 commits into from
Mar 24, 2022

Conversation

surajnpn
Copy link
Contributor

@surajnpn surajnpn commented Mar 22, 2022 •
edited

timex.plugin has only been available to Linux systems. This commit enables timex plugin to FreeBSD and MacOS.

Summary
Test Plan

Tested in FreeBSD and Linux as follows:
Built with: ./netdata-installer.sh --install /tmp
Verified with the following apis:

  • /api/v1/data?chart=netdata.plugin_timex_dt
  • /api/v1/data?chart=netdata.plugin_timex
  • /api/v1/data?chart=system.clock_sync_offset
  • /api/v1/data?chart=system.clock_sync_state

Need some volunteer to test in MacOS

Additional Information

@surajnpn surajnpn changed the title timex: this plugin enables timex plugin for non-linux systems timex: this plugin enables timex plugin for non-linux systems Closes #12437 Mar 22, 2022
@vkalintiris
Copy link
Contributor

If the timex plugin becomes available across all of linux, freebsd and macos, we should:

  1. Just add its sources to the NETDATA_FILES variable in CMakeLists.txt and Makefile.am.
  2. Add an entry to the static_threads_common array in daemon/static_threads.c.

@github-actions github-actions bot added area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/daemon labels Mar 22, 2022
@surajnpn surajnpn changed the title timex: this plugin enables timex plugin for non-linux systems Closes #12437 WIP: timex: this plugin enables timex plugin for non-linux systems Closes #12437 Mar 23, 2022
@surajnpn surajnpn marked this pull request as draft March 23, 2022 09:16
@surajnpn surajnpn changed the title WIP: timex: this plugin enables timex plugin for non-linux systems Closes #12437 timex: this plugin enables timex plugin for non-linux systems Closes #12437 Mar 23, 2022
@surajnpn surajnpn marked this pull request as ready for review March 23, 2022 09:19
vkalintiris
vkalintiris reviewed Mar 23, 2022
View reviewed changes
collectors/timex.plugin/plugin_timex.c Outdated
Comment on lines 48 to 52
#if defined(__FreeBSD__) || defined(__APPLE__)
sync_state = ntp_adjtime(&timex_buf);
#else
sync_state = adjtimex(&timex_buf);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, move this code under a new function in libnetdata/os.c (something like adjust_time() or something similar).

We want to reduce the amount of OS-specific macros outside of libnetdata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point..I'll make changes

Copy link
Contributor Author

@surajnpn surajnpn Mar 23, 2022 •
edited

Choose a reason for hiding this comment

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

I think aliasing something like below in os.h should be ok in this use case.

#include <sys/timex.h>
#if defined(__FreeBSD__) || defined(__APPLE__)
#define ADJUST_TIMEX(x) ntp_adjtime(x)
#else
#define ADJUST_TIMEX(x) adjtimex(x)
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Better to make this a function (you can pass it around, error messages match the real name, etc.). Either way it's fine by me, but make sure we use a lower-case name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, it's better to use CAPS with macros so as to distinguish them with regular variables/functions

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, it's better to use CAPS with macros so as to distinguish them with regular variables/functions

Agreed, but the rest of the code already uses lowercase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but the rest of the code already uses lowercase.

Not everywhere. Even in this file, we use uppercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is partly true, I have seen both lowercase and uppercase macros in our code. But if you see the os.h itself where I've made changes, other macros are in uppercase. I still think, it's better to go with uppercase macros.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but the rest of the code already uses lowercase.

Not everywhere. Even in this file, we use uppercase.

...for function-like macros.

We are debating a trivial thing here. For the time being, this will be used only by timex. Let's pick the approach you prefer and merge the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this is really trivial thing. lets merge this PR for now.

@vkalintiris
Copy link
Contributor

@surajnpn thanks for the changes, this LGTM. There's one last change before merging this, see comment inline.

@ilyam8
Copy link
Member

ilyam8 commented Mar 23, 2022

@surajnpn one more minor thing we need to change (not Linux specific after this PR)

netdata/collectors/timex.plugin/README.md

Line 9 in 1023c3e

This plugin monitors the system clock synchronization state on Linux nodes.

@surajnpn surajnpn requested a review from Ferroin March 23, 2022 14:16
@surajnpn surajnpn requested a review from ilyam8 March 24, 2022 09:51
@surajnpn surajnpn changed the title timex: this plugin enables timex plugin for non-linux systems Closes #12437 timex: this plugin enables timex plugin for non-linux systems Mar 24, 2022
@surajnpn surajnpn linked an issue Mar 24, 2022 that may be closed by this pull request
[Feat]: use ntp_adjtime instead of adjtimex in timex.plugin #12437
Closed
@surajnpn surajnpn merged commit 06300b1 into netdata:master Mar 24, 2022
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request May 17, 2022
@bmwiedemann
Update netdata to version 1.34.1 / rev 33 via SR 977544
7627c1b 
https://build.opensuse.org/request/show/977544
by user mia + dimstar_suse
- Update to 1.34.1 (go.d.plugin 0.32.3)
Collectors
  * New collectors
    + Add CPU throttling charts (cgroups.plugin)
      (gh#netdata/netdata#12591)
    + Add clock status chart (timex.plugin)
      (gh#netdata/netdata#12501)
    + Add Asterisk configuration file with synthetic charts
      (statsd.plugin)
      (gh#netdata/netdata#12381)
    + Add new chart for process states metrics (apps.plugin)
      (gh#netdata/netdata#12305)
    + Add thermal zone metrics collection (go.d/wmi)
      (gh#netdata/netdata#667)
    + Add SNMP data collector (go.d/snmp)
      (gh#netdata/netdata#644)
  * Improvements
    + Add 'locust' to apps_groups.conf
      (gh#netdata/netdata#12498)
    + Enable timex plugin for non-linux systems (timex.plugin)
      (gh#netdata/netdata#12489)
    + Prefer 'blkio.*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlvkobal vlvkobal vlvkobal approved these changes

@vkalintiris vkalintiris vkalintiris approved these changes

@thiagoftsm thiagoftsm Awaiting requested review from thiagoftsm

@Ferroin Ferroin Awaiting requested review from Ferroin Ferroin is a code owner

@ilyam8 ilyam8 Awaiting requested review from ilyam8

Assignees
No one assigned
Labels
area/build Build system (autotools and cmake). area/collectors Everything related to data collection area/daemon area/docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feat]: use ntp_adjtime instead of adjtimex in timex.plugin
6 participants
@surajnpn @vkalintiris @ilyam8 @vlvkobal @thiagoftsm @kickoke