Add `add_observer_metadata processor` #11394

andrewvc · 2019-03-22T17:10:08Z

Resolves #11379 via addition of new add_observer_metadata processor.

In addition to creating the processor this PR extracts out the common operations between add_observer_metadata and add_host_metadata for geo and netinfo fields into a new processors/util package.

Please note that the observer ECS field does not contain the same values that host does. See the ECS Observer Spec for more info.

elasticmachine · 2019-03-22T17:10:10Z

Pinging @elastic/uptime

libbeat/processors/util/geo.go

libbeat/processors/add_observer_metadata/config.go

libbeat/processors/add_observer_metadata/add_observer_metadata_test.go

libbeat/processors/add_observer_metadata/add_observer_metadata.go

libbeat/processors/util/netinfo.go

ruflin

argh, forgot to send my comment I made some time ago :-()

ruflin · 2019-03-25T09:40:47Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+		return nil, err
+	}
+
+	event.Fields.DeepUpdate(p.data.Get().Clone())


Not directly related to this PR but something in general I would like to see for the add_metadata_processors is that data is only added when no observer.* fields exist. If someone wants to overwrite these fields, overwrite: true has to best se. We already hit the issue in cloud and host.

It does not exist yet in other processors but as this one is new, we can introduce it from day one.

Added in the latest commits

ruflin · 2019-03-27T08:17:40Z

@elastic/apm-server Would be great to get your feedback on this one and if it would be useful for you too.

graphaelli · 2019-03-27T18:05:47Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+	data := common.MapStr{
+		"observer": common.MapStr{
+			"hostname": hostInfo.Hostname,
+			"type":     "heartbeat",


should be the beat.Info.Beat?

Yes! Good catch

graphaelli · 2019-03-27T18:17:30Z

The APM UI requires that docs have some specific fields in observer. so we can't use this directly due to the overwriting behavior, but the idea of observer metadata is interesting. apm-server does not currently expose or support use of processors though.

ruflin · 2019-03-28T08:04:37Z

@graphaelli What are the fields you require which are not provided as part of the processor? Would be great if we could get this processor to a state, where it's also useful for apm-server. I think apm-server and heartbeat have quite a few similarities.

graphaelli · 2019-03-28T13:18:52Z

We've added a few non-standard fields under observer, major_version is required by the APM UI. To be clear, I wouldn't expect these to be added to the processor, but provide support to your comment about existing observer fields taking precedence over those added by this processor.

andrewvc · 2019-03-28T18:01:55Z

@graphaelli @ruflin I've removed the vendor and type fields because processors don't have access to the beat name, and I'd rather not start down that path unless we can provide all the data. For now, I think the agent field suffices.

I'm glad to add that data, but that would be a refactor of the processor interface to take beat info with it, which may be beyond the scope here.

ruflin · 2019-04-01T06:58:44Z

This will need a rebase on master to fix the CI issues.

ruflin · 2019-04-01T06:59:40Z

libbeat/docs/processors-using.asciidoc

@@ -1054,7 +1055,7 @@ It has the following settings:


 The `add_host_metadata` processor annotates each event with relevant metadata from the host machine.
-The fields added to the event are looking as following:
+The fields added to the event are look like following:


Suggested change

The fields added to the event are look like following:

The fields added to the event look like following:

ruflin · 2019-04-01T07:00:25Z

libbeat/docs/processors-using.asciidoc

+    ],
+    "mac" : [
+      "dc:c1:02:6f:1b:ed",
+   ],


Nit: Looks like some indentation is off here.

ruflin · 2019-04-01T07:03:59Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+	keyExists, _ := event.Fields.HasKey("observer")
+
+	if p.config.Overwrite || !keyExists {
+		event.Fields.DeepUpdate(p.data.Get().Clone())


Now reading the code I wonder if in case we have overwrite: true if we should "extend" the object or actually completely overwrite it, meaning first delete observer. If we don't delete, we have the risk of having mixed data in the observer object.

I agree, overwrite should not extend, I'll patch this to delete first.

ruflin · 2019-04-01T07:04:44Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+		return nil, err
+	}
+
+	keyExists, _ := event.Fields.HasKey("observer")


We should check if HasKey also works in case an event has observer.foo instead observer: {foo: ...} as the data.

Looking at the source that appears to be the case.

@kaiyan-sheng I remember we had some discussions around this in an other PR related to host metadata.

ruflin · 2019-04-01T07:05:59Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+	if p.lastUpdate.Add(p.config.CacheTTL).After(time.Now()) {
+		return false
+	}
+	p.lastUpdate.Time = time.Now()


In the case of CacheTTL <= 0 I would argue the lastUpdate.Time should still be updated.

This logic is identical to that in add_host_metadata (indeed the code is copied). Maybe we should extract this to a new issue and patch it in both if that's a change we want to make?

ruflin · 2019-04-01T07:06:33Z

libbeat/processors/add_observer_metadata/add_observer_metadata_test.go

+
+func TestOverwriteFalse(t *testing.T) {
+	event := &beat.Event{
+		Fields:    common.MapStr{"observer": common.MapStr{"foo": "bar"}},


Can you check here with observer.foo if it still works?

That does not work because the literal map syntax does the wrong thing. If you instantiate a common.MapStr then myms.Put("observer.foo", "bar") that does work.

we have the same issue in host processor and for now if I remember correctly the conclusion is: We accept that it does not work.

ruflin · 2019-04-01T07:07:22Z

libbeat/processors/processor.go

@@ -71,7 +71,12 @@ func New(config PluginConfig) (*Processors, error) {

 		gen, exists := registry.reg[actionName]
 		if !exists {
-			return nil, errors.Errorf("the processor action %s does not exist", actionName)
+			var validActions []string


How is this change related?

I made a mistake in earlier versions of this and found this debugging info useful. Glad to move it to a new PR if you feel it's worthwhile.

libbeat/processors/script/javascript/module/processor/processor.go

ruflin · 2019-04-01T07:08:05Z

libbeat/processors/util/geo.go

+}
+
+// GeoConfigToMap converts `geo` sections to a `common.MapStr`.
+func GeoConfigToMap(config GeoConfig) (common.MapStr, error) {


I assume this code stayed the same from before.

andrewvc · 2019-04-23T02:36:31Z

@ruflin I believe I've incorporated all PR feedback here, except for one small thing about extra debugging info.

ruflin

LGTM except for the changelog entry.

CHANGELOG.next.asciidoc

ruflin · 2019-04-23T09:03:41Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+		return nil, err
+	}
+
+	keyExists, _ := event.Fields.HasKey("observer")


@kaiyan-sheng I remember we had some discussions around this in an other PR related to host metadata.

ruflin · 2019-04-23T09:04:44Z

libbeat/processors/add_observer_metadata/add_observer_metadata.go

+	if p.lastUpdate.Add(p.config.CacheTTL).After(time.Now()) {
+		return false
+	}
+	p.lastUpdate.Time = time.Now()


ruflin · 2019-04-23T09:07:09Z

libbeat/processors/add_observer_metadata/add_observer_metadata_test.go

+
+func TestOverwriteFalse(t *testing.T) {
+	event := &beat.Event{
+		Fields:    common.MapStr{"observer": common.MapStr{"foo": "bar"}},


we have the same issue in host processor and for now if I remember correctly the conclusion is: We accept that it does not work.

ruflin · 2019-04-23T09:08:33Z

libbeat/processors/processor.go

@@ -71,7 +71,12 @@ func New(config PluginConfig) (*Processors, error) {

 		gen, exists := registry.reg[actionName]
 		if !exists {
-			return nil, errors.Errorf("the processor action %s does not exist", actionName)
+			var validActions []string


ruflin · 2019-04-23T09:09:20Z

libbeat/processors/util/geo.go

+// specific language governing permissions and limitations
+// under the License.
+
+package util


This is the only package inside processors which is not a processor itself. I wonder if this could cause some problems around automation like fields collection etc.

Resolves elastic#11379 via addition of new add_observer_metadata processor. In addition to creating the processor this PR extracts out the common operations between add_observer_metadata and add_host_metadata for geo and netinfo fields into a new processors/util package. Please note that the observer ECS field does not contain the same values that host does. See the ECS Observer Spec for more info. (cherry picked from commit 1d94462)

andrewvc added enhancement in progress Pull request is currently in progress. libbeat Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Mar 22, 2019

andrewvc self-assigned this Mar 22, 2019

andrewvc requested a review from a team as a code owner March 22, 2019 17:10

houndci-bot reviewed Mar 22, 2019

View reviewed changes

andrewvc force-pushed the add-observer-metadata branch from 4fa10fb to cf3d1ae Compare March 22, 2019 20:52

andrewvc requested a review from a team as a code owner March 22, 2019 21:54

houndci-bot reviewed Mar 22, 2019

View reviewed changes

libbeat/processors/add_observer_metadata/add_observer_metadata.go Show resolved Hide resolved

houndci-bot reviewed Mar 22, 2019

View reviewed changes

libbeat/processors/util/netinfo.go Outdated Show resolved Hide resolved

andrewvc added review and removed in progress Pull request is currently in progress. labels Mar 22, 2019

andrewvc mentioned this pull request Mar 22, 2019

Bringing back host metadata as observer #11379

Closed

andrewvc requested a review from ruflin March 25, 2019 20:53

ruflin reviewed Mar 27, 2019

View reviewed changes

graphaelli reviewed Mar 27, 2019

View reviewed changes

ruflin reviewed Apr 1, 2019

View reviewed changes

ruflin mentioned this pull request Apr 3, 2019

Change add_cloud_metadata to not overwrite cloud field #11612

Merged

andrewvc force-pushed the add-observer-metadata branch from b6927fa to 3ac15ef Compare April 23, 2019 02:24

ruflin approved these changes Apr 23, 2019

View reviewed changes

andrewvc added 18 commits April 23, 2019 17:23

Add `add_observer_metadata processor

fc24248

Fix formatting

3d0a4d6

Tidy up

e830a78

Further

c659cc6

Add docs

ae2b645

Make hound happy

9e99003

Add changelog

5da12e4

Fix tests

8c614f3

Extra NetInfo to separate fn

873eed4

Make hound happy

40a0073

Add license header

cdf7929

Fix python style

c02d63c

Make update

6b68562

Add overwrite option to processor

9e99f79

Remove observer.metadata and observer.type

d812170

Improve docs

676df59

Incorporate PR feedback

c279607

Fix CHANGELOG

83035e2

andrewvc force-pushed the add-observer-metadata branch from a9a63c4 to 83035e2 Compare April 23, 2019 22:25

Fix whitespace

d635c88

ruflin approved these changes Apr 24, 2019

View reviewed changes

andrewvc merged commit 1d94462 into elastic:master Apr 24, 2019

andrewvc deleted the add-observer-metadata branch April 24, 2019 13:44

andrewvc mentioned this pull request Apr 24, 2019

Cherry-pick #11394 to 7.x: Add add_observer_metadata processor #11919

Closed

andrewvc added the v7.2.0 label Apr 24, 2019

	The fields added to the event are look like following:
	The fields added to the event look like following:

Add add_observer_metadata processor #11394

Add add_observer_metadata processor #11394

Conversation

andrewvc commented Mar 22, 2019 • edited Loading

elasticmachine commented Mar 22, 2019

ruflin left a 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

ruflin commented Mar 27, 2019

graphaelli Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graphaelli commented Mar 27, 2019

ruflin commented Mar 28, 2019

graphaelli commented Mar 28, 2019 • edited Loading

andrewvc commented Mar 28, 2019

ruflin commented Apr 1, 2019

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvc commented Apr 23, 2019

ruflin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `add_observer_metadata processor` #11394

Add `add_observer_metadata processor` #11394

andrewvc commented Mar 22, 2019 •

edited

Loading

graphaelli Mar 27, 2019 •

edited

Loading

graphaelli commented Mar 28, 2019 •

edited

Loading