scraperhelper: use common scraping interface (#3487)

jrcamp · web-flow · commit 40d6a556f101 · 2021-06-28T11:07:24.000-07:00
* scraperhelper: use common scraping interface This removes the MetricsScraper and ResourceMetricsScraper interfaces in favor of a single Scraper interface that scrapes and returns pdata.Metrics. There were already helper wrappers that allow users to return MetricSlices and ResourceMetricSlices. These helpers package those results into pdata.Metrics for the base interface. Resolves #3085: This also removes the multiMetricScraper which wrapped multiple scrapers to appear as a single scraper but then functions like scraper.ID() would be empty. This was evident when trying to log errors and you couldn't figure out what scraper made the error because it was wrapped in the multiscraper. * review updates
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+## 🧰 Bug fixes 🧰
+
+- `scraperhelper`: Include the scraper name in log messages (#3487)
+
 ## v0.29.0 Beta
 
 ## 🛑 Breaking changes 🛑
diff --git a/receiver/hostmetricsreceiver/factory.go b/receiver/hostmetricsreceiver/factory.go
@@ -124,7 +124,7 @@ func createAddScraperOptions(
 		}
 
 		if ok {
-			scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddMetricsScraper(hostMetricsScraper))
+			scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddScraper(hostMetricsScraper))
 			continue
 		}
 
@@ -134,7 +134,7 @@ func createAddScraperOptions(
 		}
 
 		if ok {
-			scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddResourceMetricsScraper(resourceMetricsScraper))
+			scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddScraper(resourceMetricsScraper))
 			continue
 		}
 
@@ -144,7 +144,7 @@ func createAddScraperOptions(
 	return scraperControllerOptions, nil
 }
 
-func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ScraperFactory) (scraper scraperhelper.MetricsScraper, ok bool, err error) {
+func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ScraperFactory) (scraper scraperhelper.Scraper, ok bool, err error) {
 	factory := factories[key]
 	if factory == nil {
 		ok = false
@@ -156,7 +156,7 @@ func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key strin
 	return
 }
 
-func createResourceMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ResourceScraperFactory) (scraper scraperhelper.ResourceMetricsScraper, ok bool, err error) {
+func createResourceMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ResourceScraperFactory) (scraper scraperhelper.Scraper, ok bool, err error) {
 	factory := factories[key]
 	if factory == nil {
 		ok = false
diff --git a/receiver/hostmetricsreceiver/hostmetrics_receiver_test.go b/receiver/hostmetricsreceiver/hostmetrics_receiver_test.go
@@ -213,32 +213,32 @@ type mockFactory struct{ mock.Mock }
 type mockScraper struct{ mock.Mock }
 
 func (m *mockFactory) CreateDefaultConfig() internal.Config { return &mockConfig{} }
-func (m *mockFactory) CreateMetricsScraper(context.Context, *zap.Logger, internal.Config) (scraperhelper.MetricsScraper, error) {
+func (m *mockFactory) CreateMetricsScraper(context.Context, *zap.Logger, internal.Config) (scraperhelper.Scraper, error) {
 	args := m.MethodCalled("CreateMetricsScraper")
-	return args.Get(0).(scraperhelper.MetricsScraper), args.Error(1)
+	return args.Get(0).(scraperhelper.Scraper), args.Error(1)
 }
 
 func (m *mockScraper) ID() config.ComponentID                      { return config.NewID("") }
 func (m *mockScraper) Start(context.Context, component.Host) error { return nil }
 func (m *mockScraper) Shutdown(context.Context) error              { return nil }
-func (m *mockScraper) Scrape(context.Context, config.ComponentID) (pdata.MetricSlice, error) {
-	return pdata.NewMetricSlice(), errors.New("err1")
+func (m *mockScraper) Scrape(context.Context, config.ComponentID) (pdata.Metrics, error) {
+	return pdata.NewMetrics(), errors.New("err1")
 }
 
 type mockResourceFactory struct{ mock.Mock }
 type mockResourceScraper struct{ mock.Mock }
 
 func (m *mockResourceFactory) CreateDefaultConfig() internal.Config { return &mockConfig{} }
-func (m *mockResourceFactory) CreateResourceMetricsScraper(context.Context, *zap.Logger, internal.Config) (scraperhelper.ResourceMetricsScraper, error) {
+func (m *mockResourceFactory) CreateResourceMetricsScraper(context.Context, *zap.Logger, internal.Config) (scraperhelper.Scraper, error) {
 	args := m.MethodCalled("CreateResourceMetricsScraper")
-	return args.Get(0).(scraperhelper.ResourceMetricsScraper), args.Error(1)
+	return args.Get(0).(scraperhelper.Scraper), args.Error(1)
 }
 
 func (m *mockResourceScraper) ID() config.ComponentID                      { return config.NewID("") }
 func (m *mockResourceScraper) Start(context.Context, component.Host) error { return nil }
 func (m *mockResourceScraper) Shutdown(context.Context) error              { return nil }
-func (m *mockResourceScraper) Scrape(context.Context, config.ComponentID) (pdata.ResourceMetricsSlice, error) {
-	return pdata.NewResourceMetricsSlice(), errors.New("err2")
+func (m *mockResourceScraper) Scrape(context.Context, config.ComponentID) (pdata.Metrics, error) {
+	return pdata.NewMetrics(), errors.New("err2")
 }
 
 func TestGatherMetrics_ScraperKeyConfigError(t *testing.T) {
diff --git a/receiver/hostmetricsreceiver/internal/scraper.go b/receiver/hostmetricsreceiver/internal/scraper.go
@@ -34,7 +34,7 @@ type ScraperFactory interface {
 
 	// CreateMetricsScraper creates a scraper based on this config.
 	// If the config is not valid, error will be returned instead.
-	CreateMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.MetricsScraper, error)
+	CreateMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.Scraper, error)
 }
 
 // ResourceScraperFactory can create a ResourceScraper.
@@ -43,7 +43,7 @@ type ResourceScraperFactory interface {
 
 	// CreateResourceMetricsScraper creates a resource scraper based on this
 	// config. If the config is not valid, error will be returned instead.
-	CreateResourceMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.ResourceMetricsScraper, error)
+	CreateResourceMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.Scraper, error)
 }
 
 // Config is the configuration of a scraper.
diff --git a/receiver/hostmetricsreceiver/internal/scraper/cpuscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/cpuscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s := newCPUScraper(ctx, cfg)
 
diff --git a/receiver/hostmetricsreceiver/internal/scraper/diskscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/diskscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s, err := newDiskScraper(ctx, cfg)
 	if err != nil {
diff --git a/receiver/hostmetricsreceiver/internal/scraper/filesystemscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/filesystemscraper/factory.go
@@ -49,7 +49,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s, err := newFileSystemScraper(ctx, cfg)
 	if err != nil {
diff --git a/receiver/hostmetricsreceiver/internal/scraper/loadscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/loadscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	logger *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s := newLoadScraper(ctx, logger, cfg)
 
diff --git a/receiver/hostmetricsreceiver/internal/scraper/memoryscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/memoryscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s := newMemoryScraper(ctx, cfg)
 
diff --git a/receiver/hostmetricsreceiver/internal/scraper/networkscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/networkscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s, err := newNetworkScraper(ctx, cfg)
 	if err != nil {
diff --git a/receiver/hostmetricsreceiver/internal/scraper/pagingscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/pagingscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s := newPagingScraper(ctx, cfg)
 
diff --git a/receiver/hostmetricsreceiver/internal/scraper/processesscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/processesscraper/factory.go
@@ -44,7 +44,7 @@ func (f *Factory) CreateMetricsScraper(
 	ctx context.Context,
 	_ *zap.Logger,
 	config internal.Config,
-) (scraperhelper.MetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	cfg := config.(*Config)
 	s := newProcessesScraper(ctx, cfg)
 
diff --git a/receiver/hostmetricsreceiver/internal/scraper/processscraper/factory.go b/receiver/hostmetricsreceiver/internal/scraper/processscraper/factory.go
@@ -47,7 +47,7 @@ func (f *Factory) CreateResourceMetricsScraper(
 	_ context.Context,
 	_ *zap.Logger,
 	cfg internal.Config,
-) (scraperhelper.ResourceMetricsScraper, error) {
+) (scraperhelper.Scraper, error) {
 	if runtime.GOOS != "linux" && runtime.GOOS != "windows" {
 		return nil, errors.New("process scraper only available on Linux or Windows")
 	}
diff --git a/receiver/scraperhelper/scraper.go b/receiver/scraperhelper/scraper.go
@@ -37,28 +37,15 @@ type baseSettings struct {
 // ScraperOption apply changes to internal options.
 type ScraperOption func(*baseSettings)
 
-// BaseScraper is the base interface for scrapers.
-type BaseScraper interface {
+// Scraper is the base interface for scrapers.
+type Scraper interface {
 	component.Component
 
 	// ID returns the scraper id.
 	ID() config.ComponentID
+	Scrape(context.Context, config.ComponentID) (pdata.Metrics, error)
 }
 
-// MetricsScraper is an interface for scrapers that scrape metrics.
-type MetricsScraper interface {
-	BaseScraper
-	Scrape(context.Context, config.ComponentID) (pdata.MetricSlice, error)
-}
-
-// ResourceMetricsScraper is an interface for scrapers that scrape resource metrics.
-type ResourceMetricsScraper interface {
-	BaseScraper
-	Scrape(context.Context, config.ComponentID) (pdata.ResourceMetricsSlice, error)
-}
-
-var _ BaseScraper = (*baseScraper)(nil)
-
 type baseScraper struct {
 	component.Component
 	id config.ComponentID
@@ -87,7 +74,7 @@ type metricsScraper struct {
 	ScrapeMetrics
 }
 
-var _ MetricsScraper = (*metricsScraper)(nil)
+var _ Scraper = (*metricsScraper)(nil)
 
 // NewMetricsScraper creates a Scraper that calls Scrape at the specified
 // collection interval, reports observability information, and passes the
@@ -96,7 +83,7 @@ func NewMetricsScraper(
 	name string,
 	scrape ScrapeMetrics,
 	options ...ScraperOption,
-) MetricsScraper {
+) Scraper {
 	set := &baseSettings{}
 	for _, op := range options {
 		op(set)
@@ -113,7 +100,7 @@ func NewMetricsScraper(
 	return ms
 }
 
-func (ms metricsScraper) Scrape(ctx context.Context, receiverID config.ComponentID) (pdata.MetricSlice, error) {
+func (ms metricsScraper) Scrape(ctx context.Context, receiverID config.ComponentID) (pdata.Metrics, error) {
 	ctx = obsreport.ScraperContext(ctx, receiverID, ms.ID())
 	scrp := obsreport.NewScraper(obsreport.ScraperSettings{ReceiverID: receiverID, Scraper: ms.ID()})
 	ctx = scrp.StartMetricsOp(ctx)
@@ -122,16 +109,20 @@ func (ms metricsScraper) Scrape(ctx context.Context, receiverID config.Component
 	if err == nil {
 		count = metrics.Len()
 	}
+	md := pdata.NewMetrics()
+	if metrics.Len() > 0 {
+		metrics.MoveAndAppendTo(md.ResourceMetrics().AppendEmpty().InstrumentationLibraryMetrics().AppendEmpty().Metrics())
+	}
 	scrp.EndMetricsOp(ctx, count, err)
-	return metrics, err
+	return md, err
 }
 
 type resourceMetricsScraper struct {
 	baseScraper
 	ScrapeResourceMetrics
 }
 
-var _ ResourceMetricsScraper = (*resourceMetricsScraper)(nil)
+var _ Scraper = (*resourceMetricsScraper)(nil)
 
 // NewResourceMetricsScraper creates a Scraper that calls Scrape at the
 // specified collection interval, reports observability information, and
@@ -140,7 +131,7 @@ func NewResourceMetricsScraper(
 	id config.ComponentID,
 	scrape ScrapeResourceMetrics,
 	options ...ScraperOption,
-) ResourceMetricsScraper {
+) Scraper {
 	set := &baseSettings{}
 	for _, op := range options {
 		op(set)
@@ -157,7 +148,7 @@ func NewResourceMetricsScraper(
 	return rms
 }
 
-func (rms resourceMetricsScraper) Scrape(ctx context.Context, receiverID config.ComponentID) (pdata.ResourceMetricsSlice, error) {
+func (rms resourceMetricsScraper) Scrape(ctx context.Context, receiverID config.ComponentID) (pdata.Metrics, error) {
 	ctx = obsreport.ScraperContext(ctx, receiverID, rms.ID())
 	scrp := obsreport.NewScraper(obsreport.ScraperSettings{ReceiverID: receiverID, Scraper: rms.ID()})
 	ctx = scrp.StartMetricsOp(ctx)
@@ -166,8 +157,14 @@ func (rms resourceMetricsScraper) Scrape(ctx context.Context, receiverID config.
 	if err == nil {
 		count = metricCount(resourceMetrics)
 	}
+
+	md := pdata.NewMetrics()
+	if resourceMetrics.Len() > 0 {
+		resourceMetrics.MoveAndAppendTo(md.ResourceMetrics())
+	}
+
 	scrp.EndMetricsOp(ctx, count, err)
-	return resourceMetrics, err
+	return md, err
 }
 
 func metricCount(resourceMetrics pdata.ResourceMetricsSlice) int {
diff --git a/receiver/scraperhelper/scrapercontroller.go b/receiver/scraperhelper/scrapercontroller.go
diff --git a/receiver/scraperhelper/scrapercontroller_test.go b/receiver/scraperhelper/scrapercontroller_test.go

Original file line number	Diff line number	Diff line change
`@@ -124,7 +124,7 @@ func createAddScraperOptions(`
`124`	`124`	`}`
`125`	`125`
`126`	`126`	`if ok {`
`127`		`- scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddMetricsScraper(hostMetricsScraper))`
	`127`	`+ scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddScraper(hostMetricsScraper))`
`128`	`128`	`continue`
`129`	`129`	`}`
`130`	`130`
`@@ -134,7 +134,7 @@ func createAddScraperOptions(`
`134`	`134`	`}`
`135`	`135`
`136`	`136`	`if ok {`
`137`		`- scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddResourceMetricsScraper(resourceMetricsScraper))`
	`137`	`+ scraperControllerOptions = append(scraperControllerOptions, scraperhelper.AddScraper(resourceMetricsScraper))`
`138`	`138`	`continue`
`139`	`139`	`}`
`140`	`140`
`@@ -144,7 +144,7 @@ func createAddScraperOptions(`
`144`	`144`	`return scraperControllerOptions, nil`
`145`	`145`	`}`
`146`	`146`
`147`		`-func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ScraperFactory) (scraper scraperhelper.MetricsScraper, ok bool, err error) {`
	`147`	`+func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ScraperFactory) (scraper scraperhelper.Scraper, ok bool, err error) {`
`148`	`148`	`factory := factories[key]`
`149`	`149`	`if factory == nil {`
`150`	`150`	`ok = false`
`@@ -156,7 +156,7 @@ func createHostMetricsScraper(ctx context.Context, logger *zap.Logger, key strin`
`156`	`156`	`return`
`157`	`157`	`}`
`158`	`158`
`159`		`-func createResourceMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ResourceScraperFactory) (scraper scraperhelper.ResourceMetricsScraper, ok bool, err error) {`
	`159`	`+func createResourceMetricsScraper(ctx context.Context, logger *zap.Logger, key string, cfg internal.Config, factories map[string]internal.ResourceScraperFactory) (scraper scraperhelper.Scraper, ok bool, err error) {`
`160`	`160`	`factory := factories[key]`
`161`	`161`	`if factory == nil {`
`162`	`162`	`ok = false`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ type ScraperFactory interface {`
`34`	`34`
`35`	`35`	`// CreateMetricsScraper creates a scraper based on this config.`
`36`	`36`	`// If the config is not valid, error will be returned instead.`
`37`		`- CreateMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.MetricsScraper, error)`
	`37`	`+ CreateMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.Scraper, error)`
`38`	`38`	`}`
`39`	`39`
`40`	`40`	`// ResourceScraperFactory can create a ResourceScraper.`
`@@ -43,7 +43,7 @@ type ResourceScraperFactory interface {`
`43`	`43`
`44`	`44`	`// CreateResourceMetricsScraper creates a resource scraper based on this`
`45`	`45`	`// config. If the config is not valid, error will be returned instead.`
`46`		`- CreateResourceMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.ResourceMetricsScraper, error)`
	`46`	`+ CreateResourceMetricsScraper(ctx context.Context, logger *zap.Logger, cfg Config) (scraperhelper.Scraper, error)`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`// Config is the configuration of a scraper.`