Add geo fields to `add_host_metadata` processor. by andrewvc · Pull Request #9392 · elastic/beats

andrewvc · 2018-12-05T04:04:46Z

** EDIT ** I've left the original issue below the break, but after discussion we added geo fields to the add_host_metadata processor instead of a new one. Original is below

This carries over from the discussion from #8620 .

This adds a new processor that lets users easily add geo fields associated with the host that created the event. You would use it like so:

processors:
  - add_host_geo:
      name: MN HQ
      location: "44.977753, -93.265015"

It's debate-able whether ECS should actually let you put these under perhaps agent.geo. That's something we should discuss here.

One other question here, should we just fold this functionality under add_host_metadata? I believe that probably makes more sense. We agreed in #8620 to make this a separate processor, but with the data nested under host, that makes less sense IMHO.

elasticmachine · 2018-12-05T04:04:48Z

Pinging @elastic/uptime

libbeat/processors/add_host_geo/add_host_geo.go

urso · 2018-12-05T19:35:31Z

libbeat/processors/add_host_geo/add_host_geo.go

nit: +1 on cloning, but from name only I would asMap expect to be a cast operation only. Caught me a little offguard when reading the argument for PutValue.

Good point, I'll rename to mapClone().

urso · 2018-12-05T19:37:54Z

libbeat/processors/add_host_geo/add_host_geo.go

I'd prefer to have validation for at least the fields that are stored as geo point in Elasticsearch here.

Ah, so we'd check the format to be a lat lon pair? I think that makes sense. Anything beyond that?

yes, at least lat-lon pair.

The others are kind free form text and validating these might require a location database, right? We even store the others as keyword, so not that important. Validating them might be too much effort here (would be nice if we can do some simple validation - regex like -, but no hard requirement for me).

Most important is that indexing won't fail due to misconfiguration of the processor + maps in kibana dashboards will work correctly, cause lat lon pairs are correct and make sense.

Validation added :)

libbeat/_meta/fields.ecs.yml

ruflin · 2018-12-06T14:06:27Z

libbeat/processors/add_host_geo/add_host_geo.go

Should we / can we do any validation for the location field to make sure we don't have ingestion errors?

ruflin · 2018-12-06T14:06:43Z

libbeat/processors/add_host_geo/add_host_geo.go

Nit but I'm somehow used to have configs on top

ruflin · 2018-12-06T14:08:45Z

libbeat/processors/add_host_geo/add_host_geo.go

Does this really belong into the host? We are interested where the agent is running so it would be agent.geo?

It's an interesting question. I agree that agent is slightly better, but I wonder if that is outweighed by the simplicity of not having yet another geo field. A location is tied to a host as much as it is tied to an agent.

There's an argument that's not true for the geo.name field we use, which is free-form, but perhaps we don't need to be so dogmatic there.

One point that would speak for the host is that in the case of apm-server, apm-server is not the agent but the observer. There it would then be observer.geo.*. Having it under host would then not change it. I'm good with going with host.* for now and if someone is not happy with that, he can use the rename processor.

+1 for not having agent.geo.*
As @ruflin says, in ECS, the agent is defined as running on a host or an observer.

Both host.geo.* and observer.geo.* are defined by ECS, so we are covered in either case.

andrewvc · 2018-12-07T14:18:57Z

After a discussion with @ruflin we decided to move this to the add_host_metadata processor

andrewvc · 2018-12-12T01:20:50Z

OK, redid the whole thing under the add_host_metadata processor.

The only thing not yet done is the changes to fields.yml. Since this is now in ECS, is there a process for updated the ECS fields, or should I just manually add the ECS name field?

libbeat/processors/add_host_metadata/config.go

ruflin

LGTM but we should add a CHANGELOG entry.

ruflin · 2018-12-12T14:11:09Z

libbeat/processors/add_host_metadata/add_host_metadata.go

I'm undecided if I like this one or not. It's good that we check the user input but it looks like really complex regexp where I must confess I could not easily review to make sure it's correct.

I can break the regexp up into component strings to make ti more readable.

FYI, the regexp isn't 100% accurate (it doesn't check the numeric bounds), but it's a lot better than nothing

In b4feb7e I've made the regex more clear

ruflin · 2018-12-12T19:39:24Z

@andrewvc Feel free to edit the ecs or the libbeat common file for the field, both works.

andrewvc · 2018-12-12T22:54:02Z

Looks like rebasing of master gives us the geo fields thanks to @andrewkroh :) in #9121

ruflin

LGTM except a missing CHANGELOG entry.

Ignore the failing mac build as I stopped them.

ruflin · 2018-12-13T10:30:51Z

libbeat/processors/add_host_metadata/add_host_metadata.go

Nice. Thanks for all the details here.

webmat

Looking great, love it.

Perhaps one more test for empty values being purged correctly, but other than this, LGTM

webmat · 2018-12-13T15:31:25Z

libbeat/processors/add_host_metadata/add_host_metadata_test.go

Nice tests.

Perhaps one more to test "Delete any empty values"? Make sure your empty values include things like empty spaces too {"city_name": " "}

@webmat I've added handling + tests for blank strings. Mind taking a look?

Yeah good stuff.

Seems like I noticed the "din expect" typo too late for this PR ;-)

This lets users add geo data if they have it to the host

ruflin

LGTM. Please ignore the failed test in Travis. Thought I fixed it but seems it still happens. Not related to this PR.

This lets users add geo data if they have it to the host (cherry picked from commit c6c4a30)

…ost_metadata) Same bug as the add_host_metadata fix: event.Fields.DeepUpdate(p.geoData) places direct references to the cached inner maps into the event. A downstream processor modifying observer.geo.* fields corrupts the cache for all subsequent events. add_observer_metadata was modeled after add_host_metadata and copied the same missing-Clone() pattern. The data path on line 94 already clones correctly: p.data.Get().Clone(). The geoData path did not. Introduced by PR elastic#5968 (Nicolas Ruflin, March 2018) and PR elastic#9392 (Andrew Cholakian, December 2018). Neither PR review discussed Clone() safety for geoData. https://claude.ai/code/session_01KRZtaDM6WsdBR2dhMpZTnD

Move the geoData.Clone() fix out of this PR. Instead: - Add BUG comment in add_host_metadata.go documenting the issue - Add reproduction filebeat configs (conditional + unconditional) - Add skipped tests (TestGeoDataNotCorruptedBetweenEvents, TestGeoDataCorruptionEndToEnd) that demonstrate the bug and describe the one-line fix - Revert add_observer_metadata.go change (same bug, same fix needed) The geoData cache corruption is a pre-existing upstream bug (since 2018, elastic/beats PRs elastic#5968 and elastic#9392). It should be filed as a separate issue with the repro configs and fixed in its own PR, since it's a different concern from the addFields performance optimization. https://claude.ai/code/session_01KRZtaDM6WsdBR2dhMpZTnD

andrewvc added enhancement discuss Issue needs further discussion. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Dec 5, 2018

andrewvc requested review from andrew-moldovan, ruflin, urso and webmat December 5, 2018 04:04

andrewvc mentioned this pull request Dec 5, 2018

Add location fields to root of config #8620

Closed

3 tasks

houndci-bot reviewed Dec 5, 2018

View reviewed changes

libbeat/processors/add_host_geo/add_host_geo.go Outdated Show resolved Hide resolved

libbeat/processors/add_host_geo/add_host_geo.go Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

andrew-moldovan removed their request for review December 5, 2018 04:59

urso reviewed Dec 5, 2018

View reviewed changes

ruflin reviewed Dec 6, 2018

View reviewed changes

webmat mentioned this pull request Dec 6, 2018

Add geo.name as the field for ad hoc location names. elastic/ecs#248

Merged

andrewvc self-assigned this Dec 7, 2018

andrewvc force-pushed the new-geo-fields branch from b36db5e to c0587c7 Compare December 12, 2018 01:21

houndci-bot reviewed Dec 12, 2018

View reviewed changes

libbeat/processors/add_host_metadata/config.go Outdated Show resolved Hide resolved

andrewvc changed the title ~~Add new processor for host geo~~ Add geo fields to add_host_metadata processor. Dec 12, 2018

ruflin reviewed Dec 12, 2018

View reviewed changes

andrewvc force-pushed the new-geo-fields branch 3 times, most recently from 36f8a04 to 16524a6 Compare December 12, 2018 22:53

ruflin reviewed Dec 13, 2018

View reviewed changes

libbeat/processors/add_host_metadata/add_host_metadata.go Outdated

Copy link
Copy Markdown

Contributor

ruflin Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks for all the details here.

webmat approved these changes Dec 13, 2018

View reviewed changes

Add geo location fields to add_host_metadata

2e35691

This lets users add geo data if they have it to the host

andrewvc force-pushed the new-geo-fields branch from 16524a6 to 2e35691 Compare December 13, 2018 21:32

Add handling for blank geo fields

526c90a

ruflin approved these changes Dec 14, 2018

View reviewed changes

andrewvc merged commit c6c4a30 into elastic:master Dec 14, 2018

andrewvc added a commit to andrewvc/beats that referenced this pull request Dec 14, 2018

Add geo fields to add_host_metadata processor. (elastic#9392)

5fc7c22

This lets users add geo data if they have it to the host (cherry picked from commit c6c4a30)

andrewvc mentioned this pull request Dec 14, 2018

Cherry-pick #9392 to 6.x: Add geo fields to add_host_metadata processor. #9565

Merged

andrewvc added the v6.6.0 label Dec 14, 2018

andrewvc added a commit that referenced this pull request Dec 17, 2018

Add geo fields to add_host_metadata processor. (#9392) (#9565)

cd232c8

This lets users add geo data if they have it to the host (cherry picked from commit c6c4a30)

Conversation

andrewvc commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 5, 2018

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

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!

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!

andrewvc commented Dec 7, 2018

Uh oh!

andrewvc commented Dec 12, 2018

Uh oh!

Uh oh!

ruflin left a comment

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!

ruflin commented Dec 12, 2018

Uh oh!

andrewvc commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmat left a comment

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!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andrewvc commented Dec 5, 2018 •

edited

Loading

andrewvc commented Dec 12, 2018 •

edited

Loading