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.

Sign up for GitHub

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

Jump to bottom

Finish ECS migration for Packetbeat #10193

Merged
merged 4 commits into from
Jan 24, 2019
Merged

Finish ECS migration for Packetbeat #10193

merged 4 commits into from
Jan 24, 2019

Conversation

andrewkroh
Copy link
Member

NOTE: This is based on some other open PRs and will be rebased after they merge. Please only review the last commit.

This should finish out the field updates for Packetbeat.

  • Assert that common ECS fields exist in system tests
  • Add aliases
  • Clean up config files
  • Remove unused fields from fields.common.yml
  • Remove time_zone from dashboards
  • Update navigation links on dashboards to include all dashboards
  • Add DNS / TLS to overview dashboard to highlight the capabilities
  • Update fields used in the documentation
  • Move RPC fields to NFS from Mongo (they were in the wrong package)

Part of #7968

@andrewkroh andrewkroh requested review from a team as code owners January 19, 2019 05:16
webmat
webmat reviewed Jan 21, 2019
View reviewed changes
Copy link
Contributor

@webmat webmat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

New content of this PR LGTM.

The comment about scale for ecs-migration.yml shouldn't be considered a blocker. It's a tweak we may put in place separately to generate better migration docs.

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
dev-tools/ecs-migration.yml
comment: >
Units changed from usec to nsec. Don't add an alias until all of Packetbeat
stops using this field.
comment: The units changed so no alias was added.
Copy link
Contributor

@webmat webmat Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ruflin I think it would be worth adding (yet another) field for entries in this file, to indicate scale difference on fields that can't be directly aliased but are still gone nonetheless. WDYT?

Most of the fields I've migrated to event.duration were ms to nano, but there was at least one which was micro to nano.

So we could have

- from: responsetime
  to: event.duration
  alias: false
  scale: 1000
  comment: >	  comment: The units changed so no alias was added.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the scale field for responsetime.

packetbeat/_meta/fields.common.yml Show resolved Hide resolved
packetbeat/docs/filtering.asciidoc Show resolved Hide resolved
packetbeat/docs/packetbeat-filtering.asciidoc Show resolved Hide resolved
packetbeat/docs/packetbeat-filtering.asciidoc Show resolved Hide resolved
webmat pushed a commit to webmat/beats that referenced this pull request Jan 23, 2019
Formalize the idea of documenting how the duration fields are migrating.
e8a0824 
Based on my suggestion over on Andrew's PR: elastic#10193 (comment)
@webmat
Copy link
Contributor

webmat commented Jan 23, 2019

@andrewkroh Ok, good for me. Ping me when this is rebased over the merged pgsql, so I can approve quickly.

@andrewkroh
Copy link
Member Author

Rebased. The pgsql commits are removed.

webmat
webmat approved these changes Jan 24, 2019
View reviewed changes
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.

Went through everything again, and LGTM.

If I understand correctly, method stays there for non-HTTP protocols that still have a "method", and path stays for paths that aren't URLs?

@webmat
Copy link
Contributor

webmat commented Jan 24, 2019

Since this is a cleanup PR after all the other ones, what's the changelog customs? Add a mention of this PR on each changelog entry? Or are the current entries enough?

@andrewkroh
Finish ECS migration for Packetbeat
f698323 
- Add aliases
- Clean up config files
- Remove unused fields from fields.common.yml
- Remove time_zone from dashboards
- Update navigation links on dashboards to include all dashboards
- Add DNS / TLS to overview dashboard to highlight the capabilities
- Update fields used in the documentation
- Move RPC fields to NFS from Mongo (they were in the wrong package)

Part of elastic#7968
@andrewkroh
Add scale to responsetime
6707e86
@andrewkroh
Copy link
Member Author

@webmat I have consolidated the changelog entries into one that points to the overall issue. The issue has links to each PR.

webmat
webmat approved these changes Jan 24, 2019
View reviewed changes
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.

Typo, then we're good

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@andrewkroh
Copy link
Member Author

If I understand correctly, method stays there for non-HTTP protocols that still have a "method", and path stays for paths that aren't URLs?

There are a few protocols populating method (amqp, pgsql, thrift, redis, mysql, memcache, dns). It probably makes sense for http to continue populating method as well. Will see about doing that.

@webmat
Copy link
Contributor

webmat commented Jan 24, 2019

Agreed on always filling method

@andrewkroh andrewkroh merged commit a1d3a9b into elastic:master Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webmat webmat webmat approved these changes

Assignees
No one assigned
Labels
Packetbeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andrewkroh @webmat