[Metricbeat] Update Couchdb to v2 #16455

sayden · 2020-02-20T15:21:32Z

What does this PR do?

Updates module to use a Strategy patern and decide between v1 and v2 Couchdb parsing. To do so, it has to do a pre HTTP request to root path / to know which version is deployed on each fetch (done in fetch to allow hot upgrades in container environment).

I have removed testdata tests because one of them was a bit unnecessary with our current testdata implementation (a timeout test which was testing testdata framework more than the module itself). The other testdata doesn't work anymore because more than one request is necessary now to check version.

Because we can only test in CI with a single version, I maintained current Dockerfile

Why is it important?

ATM, we were only parsing v1 metrics and v3 is going to be released soon. This PR introduces v2 parsing maintaining compatibility with our previous version.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~- [ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

Launch v1 and v2 containers

docker run -d --name couchdb16 -p 15984:5984 couchdb:1.6
docker run -d --name couchdb23 -p 25984:5984 -p 25986:5986 couchdb:2.3

Setup Metricbeat Couchdb module to point to both servers ./metricbeat modules enable couchdb and leave config similar to this:

- module: couchdb
  metricsets: ["server"]
  period: 10s
  hosts: ["localhost:15984", "localhost:25984/_node/_local/_stats", "localhost:25984/_node/nonode@nohost/_stats", "localhost:25986"]

Check that events from v1 and v2 are being received in, for example, Kibana. You can see version in field service.version and the service.id must be the same for all paths of v2.
Extra mile, check dashboards, they must maintain compatibility and show info of both servers.

Related issues

Closes metricbeat couchdb module not working with couch 2.x #16352
Relates CouchDB metribeat module #10354

Screenshots

I checked that dashboard still works.

mtojek · 2020-02-24T07:37:47Z

metricbeat/module/couchdb/server/server.go

+
+// Parse db version from HTTP response
+func getVersionFromInfo(info *CommonInfo) (*version, error) {
+	versionSlice := strings.Split(info.Version, ".")


I think it looks similarly to libbeat/common/version.go (semver parsing).

Good catch!

mtojek · 2020-02-24T07:51:49Z

metricbeat/module/couchdb/server/server.go

 	}, nil
 }

 // 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 {
+	info, err := m.getInfoFromCouchdbHost(m.Host())


I'm not convinced about the version checking routine being called during every Fetch(). I wonder if it's not better to determine it once and then use it. You can also revalidate it in case of an error (e.g. invalid schema) - to cover the case of potential upgrade.

Historically this was the preferred way to achieve this. Even in very heavy operations like opening connections to SQL databases. Frankly, I never liked very much the idea but I understood the implications (a long living SQL connection that may drop and leave the beat requiring a restart.

Revalidation using schema errors or any other more complex stuff is against the common "guides" (written nowhere) in Beats of trying to have explicit, easy to reason / easy to code / easy to maintain codebase. In Couchdb it's not a very good idea because the differences are very very small and can lead to unexpected results. An example are incomplete JSON returns by design or a mix of errors from the http transport layer with the database layer that can make your life a hell 😄

TLDR: I'll disagree and commit and move it to New 😉

Hmm... I wonder if it can lead to a potential problem - beats app starts before couchdb so it can't determine the API version.

Hmm... I wonder if it can lead to a potential problem - beats app starts before couchdb so it can't determine the API version.

Yes! We ran into this exact issue with a couple of the stack monitoring modules in Metricbeat and had to move some API calls from New into Fetch. See #15258 (fixed by #15270) and #15276 (fixed by #15306).

One solution I've seen in the past is caching such "infrequently changing" information for some period of time to save on checking it every Fetch() period. But that can introduce its own issues such as staleness so it might not be worth it. In general I err towards keeping it simple until it starts to be a problem.

Regarding the offline discussion - I think you can move the code determining API version just before calling the right fetcher. Both fetchers should be stateless, so you freely initialize them in New() and get rid of any unnecessary mem allocations in Fetch.

mtojek · 2020-02-24T08:09:00Z

metricbeat/module/couchdb/server/server.go

+
+	switch v.major {
+	case "1":
+		m.fetcher = &V1{}


Do you think this can be optimized to not create a new instance every time the Fetch is called?

sayden · 2020-03-10T14:31:47Z

jenkins, test this

sayden · 2020-03-16T20:31:33Z

I have done a last update by fetching on New and retrying on Fetch in case something of service unavailable on New.

Let's park the implementation here until we have any feedback from community and users before over engineering this more. The real problem here is that a JSON parsing from v1 vs v2 won't raise any error. A custom JSON parser must be implemented to prevent silent errors on hot upgrades. As this is a very short-living corner case, it's reasonable to stop here.

mtojek · 2020-03-17T08:17:52Z

metricbeat/module/couchdb/server/server.go

@@ -76,8 +95,76 @@ func (m *MetricSet) Fetch(reporter mb.ReporterV2) error {
 		return errors.Wrap(err, "error in http fetch")
 	}

-	event, _ := eventMapping(content)
-	reporter.Event(mb.Event{MetricSetFields: event})
+	if m.fetcher == nil {


nit: I would hide this condition inside retrieveFetcher. The provider creates an instance if it's not ready.

Moved it inside, thanks!

mtojek

Only one nit-pick

…sion is found

(cherry picked from commit 21be671)

sayden added enhancement Metricbeat Metricbeat Team:Services (Deprecated) Label for the former Integrations-Services team labels Feb 20, 2020

sayden requested a review from a team as a code owner February 20, 2020 15:21

sayden self-assigned this Feb 20, 2020

sayden changed the title ~~[Metricbeat] Update Couchdb to process v2 too~~ [Metricbeat] Update Couchdb to v2 Feb 20, 2020

sayden mentioned this pull request Feb 20, 2020

metricbeat couchdb module not working with couch 2.x #16352

Closed

jsoriano mentioned this pull request Feb 20, 2020

CouchDB metribeat module #10354

Closed

14 tasks

sayden force-pushed the feature/mb/couchdb-v2 branch from 55d8f8a to 08448cd Compare February 20, 2020 15:29

andresrc added [zube]: Inbox [zube]: In Review and removed [zube]: Inbox labels Feb 21, 2020

mtojek reviewed Feb 24, 2020

View reviewed changes

sayden force-pushed the feature/mb/couchdb-v2 branch from 7d09b5f to 9095c0c Compare February 28, 2020 15:39

sayden force-pushed the feature/mb/couchdb-v2 branch 3 times, most recently from 37c4662 to f3b54a8 Compare March 9, 2020 14:44

sayden force-pushed the feature/mb/couchdb-v2 branch 2 times, most recently from e62104e to d5b59c4 Compare March 16, 2020 20:16

mtojek reviewed Mar 17, 2020

View reviewed changes

mtojek self-requested a review March 17, 2020 08:18

mtojek approved these changes Mar 17, 2020

View reviewed changes

sayden force-pushed the feature/mb/couchdb-v2 branch from d5b59c4 to 230788c Compare March 17, 2020 11:12

sayden added 3 commits March 17, 2020 16:25

Atomic commit

69c7055

Update server to fetch on new and retry on fetch if error until a ver…

6983a2a

…sion is found

Move fetcher check to retrieveFetcher function

1ebf51a

sayden force-pushed the feature/mb/couchdb-v2 branch from 230788c to 1ebf51a Compare March 17, 2020 15:27

Fix broken test

489d4a3

sayden added 3 commits March 17, 2020 22:13

Reference updated

5ee8715

Remove ecs prefix

d902a1d

Update data.json

0a9f589

sayden merged commit 21be671 into elastic:master Mar 18, 2020

zube bot added [zube]: Done and removed [zube]: In Review labels Mar 18, 2020

sayden mentioned this pull request Mar 18, 2020

Cherry-pick #16455 to 7.x: [Metricbeat] Update Couchdb to v2 #17078

Merged

4 tasks

sayden added the v7.7.0 label Mar 18, 2020

sayden added a commit to sayden/beats that referenced this pull request Mar 18, 2020

[Metricbeat] Update Couchdb to support v2 too (elastic#16455)

b85ad7e

(cherry picked from commit 21be671)

sayden added a commit that referenced this pull request Mar 18, 2020

Cherry-pick #16455 to 7.x: [Metricbeat] Update Couchdb to v2 (#17078)

965a80e

andresrc removed the [zube]: Done label Mar 23, 2020

sayden added test-plan-added This PR has been added to the test plan test-plan Add this PR to be manual test plan labels Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Metricbeat] Update Couchdb to v2 #16455

[Metricbeat] Update Couchdb to v2 #16455

sayden commented Feb 20, 2020 •

edited

Loading

mtojek Feb 24, 2020

sayden Feb 27, 2020

mtojek Feb 24, 2020

sayden Feb 27, 2020

mtojek Feb 27, 2020

ycombinator Mar 11, 2020 •

edited

Loading

mtojek Mar 11, 2020

mtojek Feb 24, 2020

sayden Feb 27, 2020

sayden commented Mar 10, 2020

sayden commented Mar 16, 2020

mtojek Mar 17, 2020

sayden Mar 17, 2020

mtojek left a comment

[Metricbeat] Update Couchdb to v2 #16455

[Metricbeat] Update Couchdb to v2 #16455

Conversation

sayden commented Feb 20, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayden commented Mar 10, 2020

sayden commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

sayden commented Feb 20, 2020 •

edited

Loading

ycombinator Mar 11, 2020 •

edited

Loading