Add geo fields to `add_host_metadata` processor. #9392

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

+}
+
+func (ag addGeo) asMap() common.MapStr {
+	return common.MapStr(ag).Clone()


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

+	CountryISOCode string `config:"country_iso_code"`
+	RegionName     string `config:"region_name"`
+	CityName       string `config:"city_name"`
+}


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

+
+	ag := addGeo{
+		"name":             config.Name,
+		"location":         config.Location,


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

+	return ag, nil
+}
+
+type Config struct {


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

+type addGeo common.MapStr
+
+func (ag addGeo) Run(event *beat.Event) (*beat.Event, error) {
+	_, err := event.PutValue("host.geo", ag.asMap())


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

+
+	if config.Geo != nil {
+		if len(config.Geo.Location) > 0 {
+			if m, _ := regexp.MatchString("^\\-?\\d+(\\.\\d+)?\\s*\\,\\s*\\-?\\d+(\\.\\d+)?$", config.Geo.Location); !m {


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

+		if len(config.Geo.Location) > 0 {
+			// Regexp matching a number with an optional decimal component
+			// Valid numbers: '123', '123.23', etc.
+			latOrLon := `\-?\d+(\.\d+)?`


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)

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 webmat, urso, ruflin and andrew-moldovan 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 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

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)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add geo fields to `add_host_metadata` processor. #9392

Add geo fields to `add_host_metadata` processor. #9392

andrewvc commented Dec 5, 2018 •

edited

Loading

elasticmachine commented Dec 5, 2018

This comment has been minimized.

urso Dec 5, 2018

andrewvc Dec 6, 2018

urso Dec 5, 2018

andrewvc Dec 6, 2018

urso Dec 6, 2018

andrewvc Dec 12, 2018

ruflin Dec 6, 2018

ruflin Dec 6, 2018

ruflin Dec 6, 2018

andrewvc Dec 7, 2018

ruflin Dec 7, 2018

MikePaquette Dec 7, 2018

andrewvc commented Dec 7, 2018

andrewvc commented Dec 12, 2018

ruflin left a comment

ruflin Dec 12, 2018

andrewvc Dec 12, 2018

andrewvc Dec 12, 2018

andrewvc Dec 12, 2018

ruflin commented Dec 12, 2018

andrewvc commented Dec 12, 2018 •

edited

Loading

ruflin left a comment

ruflin Dec 13, 2018

webmat left a comment

webmat Dec 13, 2018

andrewvc Dec 14, 2018

webmat Dec 14, 2018

ruflin left a comment

Add geo fields to add_host_metadata processor. #9392

Add geo fields to add_host_metadata processor. #9392

Conversation

andrewvc commented Dec 5, 2018 • edited Loading

elasticmachine commented Dec 5, 2018

This comment has been minimized.

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 Dec 7, 2018

andrewvc commented Dec 12, 2018

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

ruflin commented Dec 12, 2018

andrewvc commented Dec 12, 2018 • edited Loading

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmat 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 left a comment

Choose a reason for hiding this comment

Add geo fields to `add_host_metadata` processor. #9392

Add geo fields to `add_host_metadata` processor. #9392

andrewvc commented Dec 5, 2018 •

edited

Loading

andrewvc commented Dec 12, 2018 •

edited

Loading