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

hostmetricsreceiver doc updates and logging #2038

Merged
merged 3 commits into from
Nov 18, 2020
Merged

hostmetricsreceiver doc updates and logging #2038

merged 3 commits into from
Nov 18, 2020

Conversation

jrcamp
Copy link
Contributor

@jrcamp jrcamp commented Oct 30, 2020

CPU and disk metrics are not available in gopsutils on Mac when cgo is disabled
(https://github.com/shirou/gopsutil/blob/master/cpu/cpu_darwin_nocgo.go)

Errors were not logged which lead to confusion about why things weren't
working. @james-bebbington was this intentional or unintentional? It does
spam the logs a bit (every collection interval) but I don't think we can fully
rely on users checking traces for errors.

@jrcamp jrcamp requested review from james-bebbington and a team October 30, 2020 15:32
@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #2038 (c26052f) into master (9bf7885) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2038      +/-   ##
==========================================
- Coverage   92.23%   92.22%   -0.01%     
==========================================
  Files         280      280              
  Lines       16996    17001       +5     
==========================================
+ Hits        15676    15679       +3     
- Misses        905      906       +1     
- Partials      415      416       +1     
Impacted Files Coverage Δ
receiver/hostmetricsreceiver/factory.go 86.84% <100.00%> (+0.17%) ⬆️
receiver/receiverhelper/scrapercontroller.go 98.93% <100.00%> (+0.04%) ⬆️
translator/internaldata/resource_to_oc.go 91.54% <0.00%> (-2.82%) ⬇️

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 9bf7885...c26052f. Read the comment docs.

@james-bebbington
Copy link
Member

james-bebbington commented Oct 31, 2020

Yea I was being very conservative re logging. This change probably makes sense, given that typical scrape intervals are once every 10s-5m.

Should we add this logging to the scraper controller instead though so that errors are logged by default for all scrapers?

@james-bebbington
Copy link
Member

james-bebbington commented Oct 31, 2020

Also, for the other part of this PR, should we disable the scraper (i.e. report an error if you try to use it) on Mac as well? (and add // +build !darwin to cpu_scraper_test.go since it currently fails if you run CGO_ENABLED=0 go test on Mac)

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 11, 2020
@tigrannajaryan
Copy link
Member

@jrcamp can you rebase?

CPU metrics are not available in gopsutils on Mac when cgo is disabled
(https://github.com/shirou/gopsutil/blob/master/cpu/cpu_darwin_nocgo.go)

Errors were not logged which lead to confusion about why things weren't
working.
Copy link
Member

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

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

LGTM

Two thoughts that don't need to be addressed in this PR:

  • It might be useful to make this logging optional?
  • Can you create an issue for disabling the cpu scraper on darwin

@jrcamp
Copy link
Contributor Author

jrcamp commented Nov 13, 2020

LGTM

Two thoughts that don't need to be addressed in this PR:

  • It might be useful to make this logging optional?

Would this be an option in ScraperControllerSettings?

  • Can you create an issue for disabling the cpu scraper on darwin

Ah yeah. Created #2143. Looks like disk also has the same problem (updated readme).

@github-actions github-actions bot removed the Stale label Nov 14, 2020
@james-bebbington
Copy link
Member

james-bebbington commented Nov 16, 2020

Would this be an option in ScraperControllerSettings?

I was thinking it's something developers would want to configure per scraper type rather than users needing to configure this directly, but maybe.

@tigrannajaryan tigrannajaryan merged commit 8b67a09 into open-telemetry:master Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants