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

(#475) Add support for host and process uptime fields #477

Merged
merged 2 commits into from
Jun 14, 2019

Conversation

mdelapenya
Copy link
Contributor

@mdelapenya mdelapenya commented May 31, 2019

Closes #475

What does this PR do?

This PR adds fields for process.uptime and server.uptime. As suggested by @webmat, the uptime field will be measured in seconds.

@ghost
Copy link

ghost commented May 31, 2019

Hi @mdelapenya, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@mdelapenya
Copy link
Contributor Author

Hey @webmat, I updated the PR to include unit suffix as first-class citizen, so the new fields are
process.uptime.seconds and server.uptime.seconds

cc/ @axw

@webmat
Copy link
Contributor

webmat commented Jun 12, 2019

@mdelapenya Thanks for updating the PR. My suggestion was to make it seconds, but I should have been more explicit: the unit shouldn't be part of the field name. We should have the description state that it's seconds.

In the context of the ecs-metrics experimental branch, we're even considering making this directive official, see #481

@mdelapenya
Copy link
Contributor Author

Oh my! I understood totally the opposite from the thread :) Also reading how prometheus folks define metrics made me think that way.

Anyway, I'll revert back to the original, where units were explicitly described in fields comments/descriptions

Thanks!

@mdelapenya
Copy link
Contributor Author

PR updated with your suggestions, thanks!

@mdelapenya mdelapenya requested a review from webmat June 13, 2019 08:57
@ruflin ruflin mentioned this pull request Jun 13, 2019
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.

Thanks for the quick adjustments. I have a few requests still, and then we'll be good.

@@ -74,3 +74,10 @@
example: 12
description: >
Packets sent from the server to the client.

- name: uptime
Copy link
Contributor

@webmat webmat Jun 13, 2019

Choose a reason for hiding this comment

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

Sorry I hadn't noticed this. The field set server is meant to describe the server side of a network connection. We have two pairs: client/server and source/destination. As such, this isn't the right place for this.

server.uptime should be moved to host.uptime :-)

Let's also make host.uptime extended for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is extended a matter of adding a level: extended, or is there any other consideration?

@@ -87,6 +87,13 @@
description: >
The time the process started.

- name: uptime
level: core
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this extended for now.

@mdelapenya
Copy link
Contributor Author

Changes ready!

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.

LGTM

Merge at your convenience :-) No need to backport or anything special.

@mdelapenya mdelapenya merged commit 20f68ca into elastic:master Jun 14, 2019
@mdelapenya mdelapenya changed the title (#475) Add support for server and process uptime fields (#475) Add support for host and process uptime fields Jun 14, 2019
@mdelapenya
Copy link
Contributor Author

Thanks @webmat!! What about the docs? Is this something I have to handle too?

@ruflin
Copy link
Contributor

ruflin commented Jun 14, 2019

@mdelapenya Docs were generated automatically.

@webmat
Copy link
Contributor

webmat commented Jun 14, 2019

Yes, generated every half hour. But note that master != current on the docs. So you can see your contribution if you go to the docs and select "master" in the version selector.

@mdelapenya mdelapenya deleted the 475-uptime branch June 14, 2019 21:34
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.

Support for host.uptime and process.uptime
3 participants