Skip to content
This repository has been archived by the owner on Mar 27, 2024. It is now read-only.

add chrony collect modules #678

Merged
merged 7 commits into from
May 25, 2022
Merged

add chrony collect modules #678

merged 7 commits into from
May 25, 2022

Conversation

boxjan
Copy link
Contributor

@boxjan boxjan commented May 1, 2022

We use go.d.plugin to collect chrony metrics in prod env so long time, and it was work very well, we think is quickly than python.d.

Test case will fix later.

We use sentry to statistics problem, I am not sure the repo will welcome it, so I didn't put it in this PR. If you have any suggest please show it.

@boxjan boxjan requested a review from ilyam8 as a code owner May 1, 2022 07:47
@ilyam8
Copy link
Member

ilyam8 commented May 3, 2022 •
edited
Loading

Hi, @boxjan. Big thanks for the contribution!

  • A small request before we start reviewing it - can you make all Public methods/functions private if they are used only in the chrony package (all except Init, Check, Collect, Cleanup).
  • And you haven't added any test. Do you plan to add them?

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2022 •
edited
Loading

Codecov Report

Merging #678 (26194ef) into master (1681b43) will decrease coverage by 0.68%.
The diff coverage is 27.01%.

@boxjan
Copy link
Contributor Author

boxjan commented May 15, 2022

Hi, @ilyam8 all Public methods/functions have been change private if they are used only in the chrony package,
about the test case, I think we can't fake a reply based on the current request, otherwise we don't know if chrony's reply can really be processed so all of these test need chrony running, and listen at default port.

@boxjan boxjan changed the title [WIP]add chrony collect modules add chrony collect modules May 15, 2022
@boxjan
Copy link
Contributor Author

boxjan commented May 15, 2022

So the test case only checked what should not be wrong in most cases

@surajnpn surajnpn merged commit c7d7fc2 into netdata:master May 25, 2022
@ilyam8
Copy link
Member

ilyam8 commented Jul 13, 2022

Hi, @boxjan. Why did you decide to not use the https://github.com/facebook/time/tree/main/ntp/chrony library directly?

@boxjan
Copy link
Contributor Author

boxjan commented Aug 11, 2022

In fact, we have been using our collector since November 2020, and we were not aware of the package at the time

@ralphm ralphm mentioned this pull request Aug 1, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

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

Assignees
No one assigned
Labels
area/conf area/docs area/modules
Projects
None yet
Milestone
No milestone
4 participants
@boxjan @ilyam8 @codecov-commenter @surajnpn