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

[Metricbeat] ReporterV2 migration meta-issue #10774

Closed
62 tasks done
sayden opened this issue Feb 15, 2019 · 10 comments
Closed
62 tasks done

[Metricbeat] ReporterV2 migration meta-issue #10774

sayden opened this issue Feb 15, 2019 · 10 comments
Assignees
Labels
meta Metricbeat Metricbeat Team:Integrations Label for the Integrations team technical debt

Comments

@sayden
Copy link
Contributor

sayden commented Feb 15, 2019

Using grep I managed to find this modules/metricset using Fetch() (common.Mapstr, error), Fetch () ([]common.Mapstr, error), Run(reporter mb.PushReporter) or Run(reporter mb.PushReporterV2)

@kaiyan-sheng
Copy link
Contributor

Do we need to regenerate the data.json file just to be sure that the migration didn't break anything?

@sayden
Copy link
Contributor Author

sayden commented Feb 20, 2019

I'm regenerating data.json files but they aren't changing at all, which is a good sign, BTW 🙂

Correction, some of them are changing https://github.com/elastic/beats/pull/10817/files#diff-5a9cef679c0694a2ace62a1ffc7159ad

@sayden
Copy link
Contributor Author

sayden commented Feb 20, 2019

Guide to migration

  • Check state of current Metricset: This is to avoid potential environment errors. Let's say that we are testing ETCD so let's run its tests first:
(python-env) metricbeat ➜ make python-env
(python-env) metricbeat ➜ source build/python-env/bin/activate
(python-env) metricbeat ➜ MODULE=etcd make test-module
# Navigate to the Metricset being migrated and
(python-env) metricbeat ➜ cd module/etcd/store
(python-env) metricbeat ➜ go test ./...
(python-env) metricbeat ➜ go test -tags=integration -data ./...

If everything is green, continue to the next step:

  • Add new logger variable for logging. In most modules we use an old logp implementation for logging and must be updated. For example for leader Metricset in ETCD module just place this at the top of the leader.go file:

var logger = logp.NewLogger("etcd.leader")

And then use logger.Debug or logger.Error instead of logp.
After merging #11106 now we have base.Logger() to use in the same way than just logger global variable.

  • Replace func (m *MetricSet) Fetch() (common.MapStr, error) { with func (m *MetricSet) Fetch(reporter mb.ReporterV2) { UPDATE: func (m *MetricSet) Fetch(reporter mb.ReporterV2) error {. Most of the times that function is in a file with the same name than the Metricset (file leader.go in ETCD leader Metricset, for example). At the same time, some Fetch methods didn't have comments so the default one is used (pasting here for quick reference):
    For example, a Fetch method like this:
func (m *MetricSet) Fetch() (common.MapStr, error) {
	content, err := m.http.FetchContent()
	if err != nil {
		return nil, err
	}
	return eventMapping(content), nil
}

Will end up looking like this:

// Fetch methods implements the data gathering and data conversion to the right
// format. It publishes the event which is then forwarded to the output. In case
// of an error set the Error field of mb.Event or simply call report.Error().
func (m *MetricSet) Fetch(reporter mb.ReporterV2) error {
	content, err := m.http.FetchContent()
	if err != nil {
		return err
	}

	reporter.Event(mb.Event{
		MetricSetFields: eventMapping(content),
	})
}

We can see that 3-liner of logger-report-return that we are trying to avoid but if all modules have the same 3-liner, maybe we can make something to change them all with a script and start using #10727

  • Replace TestFetch code in integration tests: Often we will find this code (taken from PostgreSQL bgwriter) that declares an event variable to assert against it:
func TestFetch(t *testing.T) {
	compose.EnsureUp(t, "postgresql")

	f := mbtest.NewEventFetcher(t, getConfig())
	event, err := f.Fetch()
	if !assert.NoError(t, err) {
		t.FailNow()
	}

	t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event)

	assert.Contains(t, event, "checkpoints")
	assert.Contains(t, event, "buffers")
	assert.Contains(t, event, "stats_reset")

        //continue...

So look for the line f := mbtest.NewEventFetcher(t, getConfig()) and from there, replace with:

	f := mbtest.NewReportingMetricSetV2Error(t, getConfig())
	events, errs := mbtest.WriteEventsReporterV2Error(f)
	if len(errs) > 0 {
		t.Fatalf("Expected 0 error, had %d. %v\n", len(errs), errs)
	}
	assert.NotEmpty(t, events)
	event := events[0].MetricSetFields

	// test continue untouched...

getConfig() might be also called getConfig(string) or getConfigWithData() or any other variant of it. Just check on the same file because you'll find that function somewhere there.

  • Replace TestData function: TestData can be almost fully replaced with a copy-paste. Just change the name of the service that compose is checking in compose.EnsureUp and check getConfig() call like in the previous point: (UPDATE: It's better to not use Compose here because Fetch function is the only one that should start a container in CI, this function is only to run in local)
func TestData(t *testing.T) {
	f := mbtest.NewReportingMetricSetV2Error(t, getConfig())

	if err := mbtest.WriteEventsReporterV2Error(f, t, ""); err != nil {
		t.Fatal("write", err)
	}
}
  • Check for uses in Unit Tests and replace them too: I have also found that some unit tests files use (or files without the _integration name on it) might have some tests using the reporter V1 interface. For example, this was on the store_test.go of the store Metricset of ETCD module:
func TestFetchEventContent(t *testing.T) {
	absPath, err := filepath.Abs("../_meta/test/")

	response, err := ioutil.ReadFile(absPath + "/storestats.json")
	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.WriteHeader(200)
		w.Header().Set("Content-Type", "application/json;")
		w.Write([]byte(response))
	}))
	defer server.Close()

	config := map[string]interface{}{
		"module":     "etcd",
		"metricsets": []string{"store"},
		"hosts":      []string{server.URL},
	}
	f := mbtest.NewEventFetcher(t, config)
	event, err := f.Fetch()
	if err != nil {
		t.Fatal(err)
	}

	t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), event)
}

So the approach is the same than TestFetch function, replace everything from f := mbtest.NewEventFetcher(t, config) with this:

	f := mbtest.NewReportingMetricSetV2Error(t, config)
	events, errs := mbtest.ReportingFetchV2Error(f)
	if len(errs) > 0 {
		t.Fatalf("Expected 0 error, had %d. %v\n", len(errs), errs)
	}
	assert.NotEmpty(t, events)

	t.Logf("%s/%s event: %+v", f.Module().Name(), f.Name(), events[0])
}

Here you don't need to extract MetricsetFields from the event[0]

  • BONUS POINTS! Run tests and regenerate data.json file: Quick ref:
# Replace etcd with the module you are testing
(python-env) metricbeat ➜ MODULE=etcd make test-module
# Navigate to the Metricset being migrated and
(python-env) metricbeat ➜ cd module/etcd/store
(python-env) metricbeat ➜ go test ./...
(python-env) metricbeat ➜ go test -tags=integration -data ./...

Everything is OK if

{
    "@timestamp": "2017-10-12T08:05:34.853Z",
    "agent": {
        "hostname": "host.example.com",
        "name": "host.example.com"
    },
    "etcd": {
        "leader": {
            "followers": {},
            "leader": "8e9e05c52164694d"
        }
    },
    "event": {
        "dataset": "etcd.leader",
        "duration": 115000,
        "module": "etcd"
    },
    "metricset": {
        "name": "leader"
    },
    "service": {
        "address": "127.0.0.1:2379",
        "type": "etcd"
    }
}

So, at the end, just 2 to 4 `files are uploaded in the PR, like here #10817

@kaiyan-sheng
Copy link
Contributor

kaiyan-sheng commented Feb 20, 2019

I'm regenerating data.json files but they aren't changing at all, which is a good sign, BTW 🙂

Correction, some of them are changing https://github.com/elastic/beats/pull/10817/files#diff-5a9cef679c0694a2ace62a1ffc7159ad

Yes! Some of the changes are expected. Like adding the event part and etc.

@kaiyan-sheng
Copy link
Contributor

Sorry, clicked the wrong button...

@sayden
Copy link
Contributor Author

sayden commented Mar 6, 2019

Examples and tutorials have been updated after merging #11106

@sayden
Copy link
Contributor Author

sayden commented Mar 6, 2019

TestData function have been updated in the tutorial too

@sayden
Copy link
Contributor Author

sayden commented Mar 7, 2019

Guide updated again to use 45074a3

@sayden
Copy link
Contributor Author

sayden commented Apr 11, 2019

Thanks to everybody for helping with this migration. Good work!

@sayden sayden closed this as completed Apr 11, 2019
ruflin added a commit to ruflin/beats that referenced this issue Apr 11, 2019
After migrating all metricsets (elastic#10774) to the Reporter interfaces, the EventFetcher and EventsFetcher interface can be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Metricbeat Metricbeat Team:Integrations Label for the Integrations team technical debt
Projects
None yet
Development

No branches or pull requests

2 participants