Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration of metricset.* fields to ECS #8941

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Nov 6, 2018

  • Add event.dataset as {module}.{metricset} but keep metricset.name around for backward compatiblity.
  • Add event.duration which comes from metricset.rtt but is now in nano instead of micro seconds. event.duration will be introduce in 6.x already.
  • Rename metricset.module to event.module
  • Rename metricset.host to service.address
  • Removed metricset.namespace as this becomes event.dataset and should now always be set.

Further changes

  • Updated some of the data.json files as example
  • Migration file updated
  • Old field definitions removed
  • System tests updated

@ruflin ruflin added in progress Pull request is currently in progress. Metricbeat Metricbeat ecs labels Nov 6, 2018
}
if event.Host != "" {
info["host"] = event.Host
e.Put("service.hostname", event.Host)
Copy link
Member Author

Choose a reason for hiding this comment

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

@webmat To make things a bit more interesting, Host here contains also the Port number. For example elasticsearch:9200.

}
if event.Took > 0 {
info["rtt"] = event.Took / time.Microsecond
e.Put("event.duration", event.Took/time.Microsecond)
Copy link
Member

Choose a reason for hiding this comment

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

This is described as being in nanoseconds in ECS.

Copy link
Member Author

Choose a reason for hiding this comment

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

@spinscale What will happen if we send nano seconds data to ES versions prior 7?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, will convert it.

Also not related to nano second support in ES as it's not a timestamp.

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewkroh As this is a breaking change, I wonder if we want to keep the old field around?

Other option would be to introduce the new field in nano seconds already in 6.6, deprecate the other one and then remove it in 7.0?

Copy link
Member

Choose a reason for hiding this comment

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

The plan you describe sounds good to me. Have both in 6.6 and remove rtt in 7.0.

@ruflin ruflin mentioned this pull request Nov 14, 2018
@ruflin ruflin force-pushed the migrate-metricset branch 3 times, most recently from 675f8eb to bba0dc1 Compare November 22, 2018 09:41
alias: true
copy_to: false

# TODO: This becomes port and domain in servce?
Copy link
Member Author

Choose a reason for hiding this comment

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

@webmat Feedback welcome on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, service doesn't have domain, ip or port.

Is the intent to represent the address exposed by a service? E.g.

service.name: sshd
service.port: 22
service.ip: 10.10.10.10
service.domain: (hostname on the local network)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the host/hostname that the user configured to connect to the service to fetch the information.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of mysql this can also look like:

  hosts: ["root:secret@tcp(127.0.0.1:3306)/"]

I'm thinking to call it uri or connection. Other ideas? I'm more reverting now from splitting this one up and just keep it as defined (without credentials).

Copy link
Contributor

@webmat webmat Dec 5, 2018

Choose a reason for hiding this comment

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

I was thinking of introducing a field for these ambiguous addresses as address. It's an extremely generic word, it doesn't clash with any of the words used in the RFC that have a somewhat specific meaning like "host", "hostname", or "authority". address also kinda works for situations where e.g. NGINX or HAProxy shove a unix socket in the "IP" field.

I think "address" could also work for DSNs like the mysql example above.


# TODO: Is it really an alias? Or will we change the unit?
- from: metricset.rrt
to: event.duration
Copy link
Contributor

@webmat webmat Nov 23, 2018

Choose a reason for hiding this comment

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

Yeah if this one is changing by a factor of 1 000, I think we can't make this an alias. We'll have to manage the deprecation a bit more manually. Example:

6.x

  • Mark metricset.rtt as deprecated
  • Keep populating metricset.rtt field with microseconds
  • Start populating event.duration field with nanoseconds
  • Convert all visualizations to use new field
    • Make sure to correctly set Kibana Index Pattern units for new field

7.0

  • remove metricset.rtt

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree on the field migration path. For the dashboards we can't start to use it in 6.x as otherwise it would break with older data.

@ruflin ruflin added the Team:Integrations Label for the Integrations team label Nov 27, 2018
@@ -22,6 +22,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha1...master[Check the HEAD d
*Journalbeat*

*Metricbeat*
- event.duration is now in nano and not micro seconds anymore.
Copy link
Member

Choose a reason for hiding this comment

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

s/micro seconds/microseconds/

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

}
if event.Took > 0 {
info["rtt"] = event.Took / time.Microsecond
e.Put("event.duration", event.Took/time.Microsecond)
Copy link
Member

Choose a reason for hiding this comment

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

The plan you describe sounds good to me. Have both in 6.6 and remove rtt in 7.0.

@ruflin ruflin force-pushed the migrate-metricset branch 2 times, most recently from c5bbe41 to 382b032 Compare December 5, 2018 07:37
@ruflin ruflin added review and removed in progress Pull request is currently in progress. labels Dec 5, 2018
@ruflin ruflin changed the title [WIP] Migration of metricset.* fields to ECS Migration of metricset.* fields to ECS Dec 5, 2018
@ruflin
Copy link
Member Author

ruflin commented Dec 5, 2018

Here is the 6.x PR to introduce event.duration and event.dataset: #9393

@ruflin ruflin force-pushed the migrate-metricset branch 2 times, most recently from 410d653 to 9662a1d Compare December 10, 2018 09:14
breaking: true

- from: metricset.host
to: service.uri
Copy link
Member Author

Choose a reason for hiding this comment

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

@ruflin Note for myself: This should be service.address

@ruflin
Copy link
Member Author

ruflin commented Dec 12, 2018

jenkins, test this

@webmat
Copy link
Contributor

webmat commented Dec 12, 2018

jenkins, test this

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

I will say someone much more familiar with these tests should probably review this. I don't feel like I know enough to spot any problems or missing changes.

I did still notice things that seem wrong to me:

  • Please address the question about service.address
  • Amidst all of the changes to the test files, there are 3 removals that seem suspicious. They can't be explained by field names or values changing. Are these expected?

First point I think is a potential blocker. Second point, I'm not sure if I'm misunderstanding the situation, this may be expected and not a blocker.

metricbeat/mb/event.go Show resolved Hide resolved
metricbeat/mb/module/example_test.go Show resolved Hide resolved
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

In this case, this LGTM.

But as I said, someone more familiar with these tests should likely have a look as well.

@ruflin ruflin force-pushed the migrate-metricset branch 2 times, most recently from 558ba1a to 3459740 Compare December 13, 2018 14:05
* Add `event.dataset` as `{module}.{metricset}` but keep `metricset.name` around for backward compatiblity.
* Add `event.duration` which comes from `metricset.rtt` but is now in nano instead of micro seconds. `event.duration` will be introduce in 6.x already.
* Rename `metricset.module` to `event.module`
* Rename `metricset.host` to `service.uri`
* Removed `metricset.namespace` as this becomes `event.dataset` and should now always be set.

Further changes

* Updated some of the data.json files as example
* Migration file updated
* Old field definitions removed
* System tests updated

add event field

enable ES again

fix redis test

fix check for host field

improve changelog and add todo

change uri to address

update fields

one more update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecs Metricbeat Metricbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants