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

Add Top Level Client and Server Objects #236

Merged
merged 8 commits into from
Dec 7, 2018

Conversation

MikePaquette
Copy link
Contributor

This PR closes #63 by adding the client.* and server.* top level objects to contain fields used in connection/session level events.

Many thanks to those who expended time, energy, and passion in issue #63 along the way!

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.

This looks good.

A few points, though:

  • I think this PR should add client and server to the list of places geo is expected to be nested.
  • I think this PR should add client and server to the list of places user is expected to be nested.
  • I also think it would be helpful if the definitions the the connection endpoints mentioned their expected pair explicitly: client & server are meant to be used together, and source & destination are meant to be used together.

Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I let it to @robgil and @webmat to confirm this change.

@andrewkroh
Copy link
Member

If #179 is merged then I think we want to add the .bytes and .packets metrics for client/server.

Copy link
Contributor

@robgil robgil left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Excellent. LGTM

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@MikePaquette MikePaquette merged commit d741600 into elastic:master Dec 7, 2018
@MikePaquette MikePaquette deleted the client-server-add branch December 7, 2018 14:19
webmat added a commit that referenced this pull request Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Top level: "client" and "server"
5 participants