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 fields for Nginx and Nginx stub metricset #2

Merged
merged 2 commits into from
May 14, 2016

Conversation

mrkschan
Copy link
Owner

@mrkschan mrkschan commented May 2, 2016

No description provided.

@mrkschan
Copy link
Owner Author

mrkschan commented May 2, 2016

@ruflin Could you check if I miss anything here?

description: >
The total number of dropped client connections.
- name: requests
type: integer
Copy link

Choose a reason for hiding this comment

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

Could make sense to switch this to "long" as "integer" could overflow here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do you mean we better not to use Atoi()?

Copy link

Choose a reason for hiding this comment

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

Good point, was not thinking of the implementation itself. The question here is what nginx will report as a max value and if it will exceed int or not. We hit some issues here recently in topbeat and filebeat where we use integer and switched to long as the values got too big.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think i would keep integer here until we no longer user Atoi() :)

@ruflin
Copy link

ruflin commented May 2, 2016

LGTM

@mrkschan
Copy link
Owner Author

mrkschan commented May 2, 2016

@ruflin when I run scripts/fields_collector.py, the metricbeat/etc/fields_base.yml is not included, do I miss something?

@ruflin
Copy link

ruflin commented May 2, 2016

@mrkschan Run make collect. That should do the trick. I really have to document that ...

@mrkschan mrkschan force-pushed the metricbeat-nginx-stub-fields branch from f346579 to ed26ba2 Compare May 2, 2016 13:52
@mrkschan
Copy link
Owner Author

mrkschan commented May 2, 2016

@ruflin did so :) does this still look good?

@ruflin
Copy link

ruflin commented May 2, 2016

Definitively.

@mrkschan
Copy link
Owner Author

mrkschan commented May 2, 2016

@ruflin Do you know what break CI https://travis-ci.org/mrkschan/beats/jobs/127222650#L404?

@ruflin
Copy link

ruflin commented May 2, 2016

It seems to look for an nginx environment which doesn't exist:

build path /home/travis/gopath/src/github.com/elastic/beats/testing/environments/tests/environments/nginx

@ruflin
Copy link

ruflin commented May 3, 2016

@mrkschan We just made a bigger refactoring of metricbeat which probably also affects a few line in the nginx module (elastic#1549). Me and @andrewkroh would be happy to open a PR against your branch to make it compatible again.

@mrkschan
Copy link
Owner Author

mrkschan commented May 3, 2016

@ruflin thanks :)

@mrkschan mrkschan force-pushed the metricbeat-nginx-stub-fields branch from ee4e0cd to 5d437f5 Compare May 14, 2016 13:12
@mrkschan mrkschan merged commit 87b6226 into metricbeat-nginx-module May 14, 2016
mrkschan pushed a commit that referenced this pull request Jul 17, 2016
Update Makefile and add Travis
mrkschan pushed a commit that referenced this pull request Jul 17, 2016
* Add Nginx module with stub status metric set
* Return error if Nginx stub status cannot be parsed
* Add Nginx stub status integration test
* Add fields for Nginx and Nginx stub metricset (#2)
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.

2 participants