Add Host Metrics receiver scaffolding #848

james-bebbington · 2020-04-20T04:11:23Z

Link to tracking Issue:
Issue 847

Description:
Added initial scaffolding code for a new hostmetricsreceiver which will scrape Host Metrics (CPU, Memory, Disk, etc) based on the operating system.

This receiver supports configuration per metric type (or "Scraper"), e.g. to collect cpu, memory and disk metrics, config would look something like:

hostmetrics:
  scrape_interval: 1s
  scrapers:
    cpu:
      report_per_cpu: true
    memory:
    disk:

Not included in this PR:

Scraping implementation: Will be added in follow-up PR
End to end tests: Will be added in follow-up PR
Windows build step: A lot of this code only runs against windows (see the +build windows flag). At some stage, we'll presumably need to add a build step that ensures this code is built and tested as part of CI (running integration & e2e tests would require a windows agent, which circleci does appear to support). Likewise, may need to look at how to measure code coverage of the windows specific code.

james-bebbington · 2020-04-20T04:16:17Z

go.mod

 	github.com/grpc-ecosystem/grpc-gateway v1.11.1
 	github.com/hashicorp/consul/api v1.2.0 // indirect
 	github.com/jaegertracing/jaeger v1.17.0
+	github.com/james-bebbington/winperfcounters v0.0.0-20200417100953-e7a978334ec0


I've temporarily put this code into a separate repo: https://github.com/james-bebbington/winperfcounters - it's just a fork of https://github.com/influxdata/telegraf/tree/master/plugins/inputs/win_perf_counters (didn't want to pull in that entire repo as it includes many unrelated dependencies)

I'm not sure where this should live in the long term. One possibility is to try and get this included in gopsutil

Would try to avoid depending on a "personal" repo. At least create an issue to track this problem.

I would directly depend on telegraf instead. If and when it is included in gopsutil change the dependency. If there is a concern that brings unwanted dependency then this could be moved to the contrib repo.
Another possibility is to copy the winperfcounters as an internal package. Not sure how licensing would work out.

I would also breakdown the commit into two parts to make it easy to review.

metrics that depend on runtime (with initial frame work).

metrics that depend on winperfcounters

I would directly depend on telegraf instead. If and when it is included in gopsutil change the dependency. If there is a concern that brings unwanted dependency then this could be moved to the contrib repo.

I initially recommended not to depend on telegraf as it brings in too many dependencies. I would have the same concerns if this were in contrib as well. @bogdandrutu @tigrannajaryan thoughts?

Would try to avoid depending on a "personal" repo. At least create an issue to track this problem.

While I understand the general aversion, we also depend on many libraries in personal repos. It's the nature of git. There isn't much risk given go modules proxying.

I'm quite hesitant to pull in telegraf as per jrcamp's reasoning. Given that its only 5-6 relatively small files, I may just bring them into the collector. I've split this PR up now to make it easier to review, but I'll come back to this in the next one.

codecov-io · 2020-04-20T04:36:34Z

Codecov Report

Merging #848 into master will decrease coverage by 0.32%.
The diff coverage is 44.70%.

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
- Coverage   85.95%   85.62%   -0.33%     
==========================================
  Files         157      160       +3     
  Lines       11841    11926      +85     
==========================================
+ Hits        10178    10212      +34     
- Misses       1296     1341      +45     
- Partials      367      373       +6

Impacted Files	Coverage Δ
receiver/hostmetricsreceiver/internal/scraper.go	`0.00% <0.00%> (ø)`
receiver/hostmetricsreceiver/factory.go	`47.82% <47.82%> (ø)`
...ceiver/hostmetricsreceiver/hostmetrics_receiver.go	`48.38% <48.38%> (ø)`
service/defaultcomponents/defaults.go	`83.33% <100.00%> (+0.35%)`	⬆️
translator/internaldata/resource_to_oc.go	`70.58% <0.00%> (-5.89%)`	⬇️

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 5e3b4e2...5050a85. Read the comment docs.

bogdandrutu

I would suggest that initially you split this into 2 PRs:

Adds receiver + scraper interfaces
Adds some of the scrapers.

bogdandrutu · 2020-04-20T16:04:05Z

go.mod

 	github.com/grpc-ecosystem/grpc-gateway v1.11.1
 	github.com/hashicorp/consul/api v1.2.0 // indirect
 	github.com/jaegertracing/jaeger v1.17.0
+	github.com/james-bebbington/winperfcounters v0.0.0-20200417100953-e7a978334ec0


Would try to avoid depending on a "personal" repo. At least create an issue to track this problem.

receiver/hostmetricsreceiver/factory.go

bogdandrutu · 2020-04-20T16:08:14Z

receiver/hostmetricsreceiver/factory.go

+
+		config.Scrapers = map[string]hmcomponent.ScraperConfig{}
+
+		scrapersViperSection := componentViperSection.Sub("scrapers")


use viperSub from the config package. see there the reasons.

config.viperSub isn't exported at the moment. I can change it to be exported, but I'll do it in another PR to avoid the refactoring noise in this PR

receiver/hostmetricsreceiver/component/scraper.go

receiver/hostmetricsreceiver/factory.go

receiver/hostmetricsreceiver/component/empty_test.go

receiver/hostmetricsreceiver/example_config.yaml

receiver/hostmetricsreceiver/component/scraper.go

receiver/hostmetricsreceiver/testdata/config.yaml

jrcamp · 2020-04-20T22:09:26Z

receiver/hostmetricsreceiver/testdata/config.yaml

@@ -0,0 +1,23 @@
+receivers:
+  hostmetrics:
+    scrapers:


Is it necessary to have these under scrapers instead of being top-level?

I think it'd be more confusing to not have a header to separate any top level config from the dynamic scraper config? (both for readability and for the code which would have to figure out which keys are top level or not)

Yeah, it probably avoids some potential confusion at the expense of nesting. Though in the example config we did without the nesting it's not too confusing I think. @tigrannajaryan has a good eye for config so maybe he can chime in.

I think your way is the safer approach though. What do you think about making it collectors or collect to keep with the collection_interval naming scheme?

From what I see there is default_collection_interval at the top-level so we likely want the additional nesting for consistency. Otherwise we would have default_collection_interval and (for example) cpu as siblings, which does not seem to make sense.

If we don't need default_collection_interval then I would prefer to eliminate scraper and reduce the nesting. I glanced at the PR and cannot see how DefaultCollectionInterval is used in the code (I assume it will be in a future PR?). Is the idea that there is a "default" interval and each scraper can override this? Would it make sense to just hard-code the default and eliminate the config key?

Makes sense - I think I'll leave it as is for now though. While we could hardcode default_collection_interval (which as you correctly point out will be used in a future PR as the default interval for individual scrapers), there's likely to be other top level config that's needed, e.g. proc_path in the example config @jrcamp linked.

@jrcamp - regarding the naming, I originally called these collectors, but ended up changing to scrapers as I thought that was too confusing / broad given the name of this repo is "collector".

receiver/hostmetricsreceiver/hostmetrics_receiver.go

james-bebbington · 2020-04-21T02:20:16Z

I would suggest that initially you split this into 2 PRs:

Adds receiver + scraper interfaces

Adds some of the scrapers.

👍 I've updated this PR so it's just the first part: Receiver & Scraper interfaces

go.mod

receiver/hostmetricsreceiver/scraper/scraper.go

…indows

rghetia · 2020-04-22T04:31:52Z

receiver/hostmetricsreceiver/factory.go

+	ScraperFactories map[string]internal.Factory
+}
+
+// NewFactory creates a new factory


Suggested change

// NewFactory creates a new factory

// NewFactory creates a new factory for host metrics receiver.

rghetia · 2020-04-22T04:32:34Z

receiver/hostmetricsreceiver/factory.go

+	}
+}
+
+// Type gets the type of the Receiver config created by this Factory.


Suggested change

// Type gets the type of the Receiver config created by this Factory.

// Type returns the type of the Receiver config created by this Factory.

rghetia · 2020-04-22T04:34:55Z

receiver/hostmetricsreceiver/factory.go

+	return typeStr
+}
+
+// CustomUnmarshaler returns custom unmarshaler for this config.


Suggested change

// CustomUnmarshaler returns custom unmarshaler for this config.

// CustomUnmarshaller returns custom unmarshaller for this factory.

rghetia · 2020-04-22T04:50:07Z

receiver/hostmetricsreceiver/factory.go

+	}
+}
+
+// CreateTraceReceiver creates a trace receiver based on provided config.


I wonder if this should be
CreateTraceReceiver returns error as trace receiver is not applicable to host metrics receiver.
Other receivers also have similar issue.

rghetia · 2020-04-22T04:58:33Z

receiver/hostmetricsreceiver/hostmetrics_receiver.go

+	"github.com/open-telemetry/opentelemetry-collector/receiver/hostmetricsreceiver/internal"
+)
+
+// Receiver is the type used to handle metrics from VM metrics.


Suggested change

// Receiver is the type used to handle metrics from VM metrics.

// Receiver is the type that provides scraping host metrics.

rghetia · 2020-04-22T04:59:42Z

receiver/hostmetricsreceiver/hostmetrics_receiver.go

+	cancel   context.CancelFunc
+}
+
+// NewHostMetricsReceiver creates a new set of VM and Process Metrics


Suggested change

// NewHostMetricsReceiver creates a new set of VM and Process Metrics

// NewHostMetricsReceiver creates a host metric scraper.

rghetia · 2020-04-22T05:10:59Z

receiver/hostmetricsreceiver/testdata/config.yaml

@@ -0,0 +1,17 @@
+receivers:
+  hostmetrics:


Please add one more hostmetrics receiver with a name, for e.g. 'hostmetrics/customname'. Similar to otlp
typical use would be that one scraper is configured with 10 second interval collecting memory metrics and another one configured with 60 second interval collecting cpu metrics. Unless each category allows configuring separate interval. In that case two instance of hostmetrics should be disallowed.

rghetia · 2020-04-22T05:14:47Z

receiver/hostmetricsreceiver/testdata/config.yaml

+
+service:
+  pipelines:
+    traces:


should be metrics

Addressed your comments in the next PR: #862

…indows (open-telemetry#848)

) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update core/contrib deps * Update renamed components * Update signalfx exporter for contrib open-telemetry#5756 * Update CHANGELOG.md * Fix breaking config map and tracer changes * Account for k8s_tagger rename * Rename k8s_tagger to k8sattributes * reduce collectd-cassandra test flake * improve k8s_tagger rename test * Update hec tls config map usage * move from retracted 0.37.0 to 0.37.1 * Update changelog * Update bundled Smart Agent to v5.14.2 * Update github.com/signalfx/signalfx-agent to main Co-authored-by: Ryan Fitzpatrick <[email protected]>

james-bebbington requested review from bogdandrutu, flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners April 20, 2020 04:11

james-bebbington commented Apr 20, 2020

View reviewed changes

james-bebbington changed the title ~~Add Host Metrics receiver scaffolding and initial CPU scraper - curre…~~ Add Host Metrics receiver Apr 20, 2020

james-bebbington mentioned this pull request Apr 20, 2020

Add HostMetrics receiver scaffolding code james-bebbington/opentelemetry-collector-contrib#1

Closed

james-bebbington force-pushed the hostmetrics branch from 544ed7e to 1fba6c9 Compare April 20, 2020 04:29

james-bebbington force-pushed the hostmetrics branch from 1fba6c9 to d0bdc37 Compare April 20, 2020 05:18

bogdandrutu reviewed Apr 20, 2020

View reviewed changes

jrcamp reviewed Apr 20, 2020

View reviewed changes

james-bebbington force-pushed the hostmetrics branch 3 times, most recently from 4870074 to b02ae44 Compare April 21, 2020 02:17

james-bebbington changed the title ~~Add Host Metrics receiver~~ Add Host Metrics receiver scaffolding Apr 21, 2020

james-bebbington force-pushed the hostmetrics branch from b02ae44 to 40e2bb3 Compare April 21, 2020 07:09

bogdandrutu reviewed Apr 21, 2020

View reviewed changes

go.mod Outdated Show resolved Hide resolved

receiver/hostmetricsreceiver/scraper/scraper.go Outdated Show resolved Hide resolved

receiver/hostmetricsreceiver/scraper/scraper.go Show resolved Hide resolved

james-bebbington force-pushed the hostmetrics branch 3 times, most recently from 5050a85 to c4ba9b4 Compare April 22, 2020 02:12

Add Host Metrics receiver scaffolding - currently only supported on W…

c9c7267

…indows

james-bebbington force-pushed the hostmetrics branch from c4ba9b4 to c9c7267 Compare April 22, 2020 02:16

bogdandrutu approved these changes Apr 22, 2020

View reviewed changes

bogdandrutu merged commit 2759f27 into open-telemetry:master Apr 22, 2020

rghetia reviewed Apr 22, 2020

View reviewed changes

james-bebbington mentioned this pull request Apr 26, 2020

Added host metrics cpu scraper #862

Merged

james-bebbington mentioned this pull request May 12, 2020

Added third party code for accessing Windows performance counters #953

Merged

wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020

Add Host Metrics receiver scaffolding - currently only supported on W…

b5eb03a

…indows (open-telemetry#848)


		config.Scrapers = map[string]hmcomponent.ScraperConfig{}

		scrapersViperSection := componentViperSection.Sub("scrapers")

	// NewFactory creates a new factory
	// NewFactory creates a new factory for host metrics receiver.

	// Type gets the type of the Receiver config created by this Factory.
	// Type returns the type of the Receiver config created by this Factory.

	// CustomUnmarshaler returns custom unmarshaler for this config.
	// CustomUnmarshaller returns custom unmarshaller for this factory.

	// Receiver is the type used to handle metrics from VM metrics.
	// Receiver is the type that provides scraping host metrics.

	// NewHostMetricsReceiver creates a new set of VM and Process Metrics
	// NewHostMetricsReceiver creates a host metric scraper.

Add Host Metrics receiver scaffolding #848

Add Host Metrics receiver scaffolding #848

Uh oh!

Conversation

james-bebbington commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-bebbington commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-bebbington commented Apr 20, 2020 •

edited

Loading

codecov-io commented Apr 20, 2020 •

edited

Loading

james-bebbington commented Apr 21, 2020 •

edited

Loading

james-bebbington Apr 23, 2020 •

edited

Loading