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 host metrics #3277

Merged
merged 2 commits into from Sep 19, 2019

Conversation

@yeya24
Copy link
Contributor

commented Sep 15, 2019

Signed-off-by: yeya24 yb532204897@gmail.com

1. Why is this pull request needed and what does it do?

add metrics to hosts plugin

2. Which issues (if any) are related?

#3241

3. Which documentation changes (if any) need to be made?

Maybe a doc

4. Does this introduce a backward incompatible change or deprecation?

No

@corbot corbot bot requested a review from johnbelamaric Sep 15, 2019
@corbot

This comment has been minimized.

Copy link

commented Sep 15, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked johnbelamaric (via /OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@codecov-io

This comment has been minimized.

Copy link

commented Sep 15, 2019

Codecov Report

Merging #3277 into master will decrease coverage by 0.02%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3277      +/-   ##
==========================================
- Coverage   55.88%   55.86%   -0.03%     
==========================================
  Files         215      215              
  Lines       10936    10943       +7     
==========================================
+ Hits         6112     6113       +1     
- Misses       4366     4372       +6     
  Partials      458      458
Impacted Files Coverage Δ
plugin/hosts/setup.go 44.14% <0%> (-2.09%) ⬇️
plugin/hosts/hostsfile.go 66.08% <0%> (-1.17%) ⬇️
plugin/ready/ready.go 86.2% <100%> (ø) ⬆️
plugin/file/reload.go 69.44% <0%> (-5.56%) ⬇️
plugin/route53/route53.go 86.61% <0%> (+2.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62317c3...dc25178. Read the comment docs.

@miekg

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

can you pull out that health/overloaded.go change and put it up as another PR? Thanks

@yeya24 yeya24 force-pushed the yeya24:support-hosts-metrics branch 2 times, most recently from 44cf1a3 to 1e1cf23 Sep 15, 2019
@yeya24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

can you pull out that health/overloaded.go change and put it up as another PR? Thanks

Done.

Copy link
Member

left a comment

thanks, a bunch of comments and the README needs to be updated to reflect that these metrics are added.

plugin/hosts/hostsfile.go Show resolved Hide resolved
hostsEntries = prometheus.NewGaugeVec(prometheus.GaugeOpts{
Namespace: plugin.Namespace,
Subsystem: "hosts",
Name: "entries",

This comment has been minimized.

Copy link
@miekg

miekg Sep 16, 2019

Member

entries_count is a better name I think

Namespace: plugin.Namespace,
Subsystem: "hosts",
Name: "entries",
Help: "The number of entries in hosts file and corefile",

This comment has been minimized.

Copy link
@miekg

miekg Sep 16, 2019

Member

"The combined number of entries in the hosts and Corefile."
(and closing dot)

plugin/hosts/hostsfile.go Show resolved Hide resolved
@yeya24 yeya24 force-pushed the yeya24:support-hosts-metrics branch from 1e1cf23 to 5b87822 Sep 16, 2019
@miekg

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@yeya24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@miekg I have addressed all of you mentioned. So I think I just need to add docs in the hosts plugin' s Readme? Do I need to add any other docs?

@miekg

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@yeya24

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Done. PTAL

Copy link
Member

left a comment

1 question, otherwise lgtm

@@ -135,6 +137,8 @@ func (h *Hostsfile) readHosts() {
h.mtime = stat.ModTime()
h.size = stat.Size()

hostsEntries.WithLabelValues().Set(float64(h.inline.Len() + h.hmap.Len()))
hostsReloadTime.SetToCurrentTime()

This comment has been minimized.

Copy link
@miekg

miekg Sep 19, 2019

Member

should we use ModTime() here?

This comment has been minimized.

Copy link
@yeya24

yeya24 Sep 19, 2019

Author Contributor

Thanks for your suggestion. I have changed to use stat.ModTime(). It is more appropriate

Signed-off-by: yeya24 <yb532204897@gmail.com>
@yeya24 yeya24 force-pushed the yeya24:support-hosts-metrics branch from 5016fa6 to dc25178 Sep 19, 2019
plugin/hosts/README.md Outdated Show resolved Hide resolved
Signed-off-by: yeya24 <yb532204897@gmail.com>
@yeya24 yeya24 force-pushed the yeya24:support-hosts-metrics branch from 7c149e9 to b12c0e6 Sep 19, 2019
@miekg
miekg approved these changes Sep 19, 2019
@miekg miekg merged commit 85e6570 into coredns:master Sep 19, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 55.86% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@yeya24 yeya24 deleted the yeya24:support-hosts-metrics branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.