[Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release #9570

andrewvc · 2018-12-14T20:15:18Z

This PR migrates a number of different heartbeat fields to their ECS counterparts in the url.* namespace.

~~This now adds url.full for all monitor types. It's still WIP since the tests haven't been updated, but I wanted to makes sure what it does is correct.~~

elasticmachine · 2018-12-14T20:15:19Z

Pinging @elastic/uptime

webmat

LGTM :-)

Reviewed too fast. The mapping is good, but the work isn't done ;-)

andrewvc · 2018-12-31T23:09:59Z

@ruflin ready to have a look taken at the code even though tests are broken.

ruflin

Does this PR also need a changelog entry?

ruflin · 2019-01-07T09:40:33Z

dev-tools/ecs-migration.yml

@@ -625,3 +625,8 @@
  to: event.dataset
  alias: false
  comment: No alias mapping as field did not always exist
+
+- from: http.url


I would also expect a change to the fields.yml file that introduces this alias?

Also in the code I couldn't find where http.url is removed?

Right now it's just being duplicated and we're relying on ECS to provide that field as a non-alias.

If you're going to keep the original http.url field present and just copy its value, you should remove the entry from ecs-migration.yml.

It should still show up here if we potentially need to migrate this in the dashboards.

But in case we keep the old field around, alias: true should be set to false as we can't create an alias for the old field.

heartbeat/monitors/util.go

andrewvc · 2019-01-08T02:00:58Z

@ruflin @webmat OK, so I took this a lot further and refactored all the monitors to use the url.* namespace in ECS extensively.

This actually cleans up the code in heartbeat quite a bit, which is a good sign I think! It lets a lot of the event field population happen at a high level (before the task is started) rather than deep in the bowels of the event's execution, and lets it happen consistently with a couple simple helper functions.

This is of course a 7.0.0 only change, and breaks cases where people use fields directly. I made an effort to alias all the moved fields and document them as well.

heartbeat/_meta/fields.common.yml

dev-tools/ecs-migration.yml

andrewvc · 2019-01-08T15:26:59Z

@ruflin I believe I've addressed all of your concerns

webmat

Looking good overall.

However you shouldn't list http.url as aliased to url.full in ecs-migration.yml, if it's only copied. Unless I misunderstand something about this?

webmat · 2019-01-08T21:37:10Z

heartbeat/monitors/util_test.go

+	"github.com/elastic/beats/libbeat/common"
+)
+
+func TestURLFields(t *testing.T) {


❤️ this whole test file

webmat · 2019-01-08T21:42:50Z

dev-tools/ecs-migration.yml

@@ -625,3 +625,8 @@
  to: event.dataset
  alias: false
  comment: No alias mapping as field did not always exist
+
+- from: http.url


If you're going to keep the original http.url field present and just copy its value, you should remove the entry from ecs-migration.yml.

andrewvc · 2019-01-09T03:12:52Z

@webmat I changed this PR since I first wrote it. Now there are no copies. Just aliases, so this is a breaking change for 7.0.0.

ruflin

What I would like to see in the changelog is having pairs of field old -> new to easily follow what changed. So if someone had queries on these fields and looks at the changes, it's very easy for him to decide what to rename it to. Yes, the ecs-migration.yml would also be there for a look up but I don't expect users to check there. Also having it in the PR description would be nice as we do for other ECS migration PR's.

In metricbeat we create these data.json with example events: https://github.com/elastic/beats/blob/master/metricbeat/module/elasticsearch/index_recovery/_meta/data.json We use these also in the docs. This would be super nice for Heartbeat to have and would make it more obvious what things change in such PR's. No need to add something like this in this PR but an idea for the future.

heartbeat/_meta/fields.common.yml

heartbeat/hbtest/hbtestutil.go

heartbeat/monitors/active/http/_meta/fields.yml

heartbeat/monitors/active/tcp/_meta/fields.yml

andrewvc · 2019-01-09T15:17:21Z

@ruflin I think I've addressed all concerns here aside from the data.json. +1 to doing that at a future time.

webmat

LGTM

The field by field explicit listing of the fields migrated is way out of scope for this. We should just generate one massive (and extremely boring) documentation file for the website, to list them.

andrewvc · 2019-01-09T20:14:23Z

Thanks @webmat ! FWIW I did add that to the changelog already.

ruflin

LGTM

Left 2 more minor comments but can be addressed in a follow up PR.

Could you after merge update the PR description as at the moment it's not fully in line with what we have in the final PR.

ruflin · 2019-01-10T08:19:51Z

heartbeat/monitors/active/http/_meta/fields.yml

          description: >
            Service url used by monitor.
-          multi_fields:


The type of the field changed here because of ECS. It means we don't have the indexed field anymore (text). Should this also be mentioned somewhere?

This field encodes a full URL version of the monitor, and adds URL encodings for HTTP, TCP, and ICMP. Fix tests, unify host behavior Use all ECS URL fields Alias heartbeat fields to ECS ones Refactor URL assignment Fix failing tests Add changelog entry Add required comments Fix PY testS Improve docs Fix samples Minor improvements to migration yml Fix imports Incorporate PR feedback Don't ignore URL parse errors Make update

andrewvc · 2019-01-10T18:47:45Z

Failures unrelated

Continues the work in elastic#9570 , aliasing this field for convenience. In ECS this field is already a keyword, so this should be fine.

andrewvc added enhancement in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Dec 14, 2018

andrewvc requested a review from a team as a code owner December 14, 2018 20:15

webmat previously approved these changes Dec 14, 2018

View reviewed changes

andrewvc mentioned this pull request Dec 18, 2018

[META] Heartbeat 7.0 TODO #9017

Closed

7 tasks

andrewvc force-pushed the add-monitor-url branch from 35b54a3 to a158ec2 Compare December 31, 2018 23:09

andrewvc changed the title ~~WIP Add monitor.url move~~ [Heartbeat] Add monitor.url move Jan 1, 2019

urso removed the request for review from a team January 2, 2019 15:19

andrewvc added review and removed in progress Pull request is currently in progress. labels Jan 3, 2019

andrewvc force-pushed the add-monitor-url branch from a158ec2 to 43ba941 Compare January 4, 2019 19:37

andrewvc requested a review from a team as a code owner January 4, 2019 19:37

ruflin requested changes Jan 7, 2019

View reviewed changes

andrewvc force-pushed the add-monitor-url branch from 43ba941 to 767df2e Compare January 7, 2019 18:54

houndci-bot reviewed Jan 7, 2019

View reviewed changes

heartbeat/monitors/util.go Show resolved Hide resolved

andrewvc force-pushed the add-monitor-url branch from 767df2e to 84dcc3a Compare January 7, 2019 21:28

houndci-bot reviewed Jan 7, 2019

View reviewed changes

heartbeat/monitors/util.go Show resolved Hide resolved

ruflin reviewed Jan 8, 2019

View reviewed changes

heartbeat/_meta/fields.common.yml Show resolved Hide resolved

dev-tools/ecs-migration.yml Show resolved Hide resolved

dev-tools/ecs-migration.yml Outdated Show resolved Hide resolved

webmat reviewed Jan 8, 2019

View reviewed changes

ruflin reviewed Jan 9, 2019

View reviewed changes

heartbeat/_meta/fields.common.yml Show resolved Hide resolved

heartbeat/hbtest/hbtestutil.go Outdated Show resolved Hide resolved

heartbeat/monitors/active/http/_meta/fields.yml Show resolved Hide resolved

heartbeat/monitors/active/tcp/_meta/fields.yml Show resolved Hide resolved

andrewvc force-pushed the add-monitor-url branch from e7b2cda to 0a99c71 Compare January 9, 2019 15:11

andrewvc changed the title ~~[Heartbeat] Add monitor.url move~~ [Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release Jan 9, 2019

webmat approved these changes Jan 9, 2019

View reviewed changes

ruflin approved these changes Jan 10, 2019

View reviewed changes

andrewvc force-pushed the add-monitor-url branch from a2f49c6 to 9894b2f Compare January 10, 2019 17:24

andrewvc merged commit 529feda into elastic:master Jan 10, 2019

andrewvc added a commit to andrewvc/beats that referenced this pull request Jan 10, 2019

[Heartbeat] Alias http.url.raw to url.full

0ab6ac9

Continues the work in elastic#9570 , aliasing this field for convenience. In ECS this field is already a keyword, so this should be fine.

andrewvc mentioned this pull request Jan 10, 2019

[Heartbeat] Alias http.url.raw to url.full #10005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release #9570

[Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release #9570

andrewvc commented Dec 14, 2018 •

edited

Loading

elasticmachine commented Dec 14, 2018

webmat left a comment

andrewvc commented Dec 31, 2018

ruflin left a comment

ruflin Jan 7, 2019

andrewvc Jan 7, 2019

webmat Jan 8, 2019

ruflin Jan 10, 2019

andrewvc commented Jan 8, 2019

andrewvc commented Jan 8, 2019

webmat left a comment

webmat Jan 8, 2019

webmat Jan 8, 2019

andrewvc commented Jan 9, 2019

ruflin left a comment

andrewvc commented Jan 9, 2019

webmat left a comment •

edited

Loading

andrewvc commented Jan 9, 2019

ruflin left a comment

ruflin Jan 10, 2019

andrewvc commented Jan 10, 2019

[Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release #9570

[Heartbeat] Migrate to ECS url.* fields for 7.0.0 Release #9570

Conversation

andrewvc commented Dec 14, 2018 • edited Loading

elasticmachine commented Dec 14, 2018

webmat left a comment

Choose a reason for hiding this comment

andrewvc commented Dec 31, 2018

ruflin left a comment

Choose a reason for hiding this comment

ruflin Jan 7, 2019

Choose a reason for hiding this comment

andrewvc Jan 7, 2019

Choose a reason for hiding this comment

webmat Jan 8, 2019

Choose a reason for hiding this comment

ruflin Jan 10, 2019

Choose a reason for hiding this comment

andrewvc commented Jan 8, 2019

andrewvc commented Jan 8, 2019

webmat left a comment

Choose a reason for hiding this comment

webmat Jan 8, 2019

Choose a reason for hiding this comment

webmat Jan 8, 2019

Choose a reason for hiding this comment

andrewvc commented Jan 9, 2019

ruflin left a comment

Choose a reason for hiding this comment

andrewvc commented Jan 9, 2019

webmat left a comment • edited Loading

Choose a reason for hiding this comment

andrewvc commented Jan 9, 2019

ruflin left a comment

Choose a reason for hiding this comment

ruflin Jan 10, 2019

Choose a reason for hiding this comment

andrewvc commented Jan 10, 2019

andrewvc commented Dec 14, 2018 •

edited

Loading

webmat left a comment •

edited

Loading