-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 couchdb module #9406
Add couchdb module #9406
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Pinging @elastic/infrastructure |
Is there anything that I can do for this unsuccessful check? |
@berfinsari Try to run |
I ran |
@berfinsari you need to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, I have added some comments, my main concern is that I am not sure if we should collect all aggregated values or only current values.
@berfinsari btw, I have seen you are also the author of other modules and some community beats for the same services, out of curiosity, do you have some preferences about using specific community beats instead of Metricbeat? |
"httpd_status_codes": common.MapStr{ | ||
"200": common.MapStr{ | ||
"description": data.HttpdStatusCodes.Num200.Description, | ||
"current": data.HttpdStatusCodes.Num200.Current, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly means current
in this context? Since the last request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that is the answer of your question, but the fields provide the current, minimum and maximum, and a collection of statistical means and quantities. These statistics produce results that measure the interaction with the server since it was started.
@jsoriano At the beginning of the process, I was creating community beats for both learning the process and preparing and using it, because there is not so many requirements like the metricbeat module in community beats. When I was keeping contribute to Metricbeat module, I was also keeping create community beats. I guess it is enough to contribute to Metricbeat module. |
Sounds good, thanks a lot for your work in these modules! |
Please, also write in the docs that this module works with Couchdb and it's not tested with Couchdb 2. While they both maintain a 99% of API compatibility, the amount of metrics you can extract from Couchdb 2 is bigger than in Couchdb 1, for example the shards of the databases or the statistics about Mango queries. |
description: > | ||
Example field | ||
fields: | ||
- name: num200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fields here should be named just with the number i.e. 200
instead of num200
.
|
||
// New creates a new instance of the MetricSet. | ||
func New(base mb.BaseMetricSet) (mb.MetricSet, error) { | ||
cfgwarn.Experimental("The couchdb server metricset is experimental.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go directly to beta for these metricsets, please update it in the fields files too.
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
const testFile = "../_meta/test/serverstats.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used variable.
@@ -0,0 +1,43 @@ | |||
"couchdb": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an incomplete JSON? It should start by {
. The error in travis seems caused by this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may have overlooked this field. I made the correction. I think the error in travis isn't about this. CouchDB returns an error because database does not exist in Docker. This causes the TestFetch() methods in integration tests to fail. I'm trying to fix this error, but I haven't found a solution yet.
|
||
func eventMapping(content []byte) common.MapStr { | ||
var data Server | ||
json.Unmarshal(content, &data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for errors here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review. I believe I've addressed all the feedback now.
@@ -0,0 +1,58 @@ | |||
{ | |||
"@timestamp":"2016-05-23T08:05:34.853Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the indentation is off in this file and contains some old fields, I wonder if this was generated or manually created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was generated. Should I update this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can you generated it again to see if the strange indentation disappears?
Number of HTTP COPY requests | ||
|
||
- name: HEAD | ||
type: scaled_float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these header counters(?) be long instead of float?
HTTP status codes statistics | ||
fields: | ||
- name: "200" | ||
type: scaled_float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above: long instead of float?
couchdb statistics | ||
fields: | ||
- name: database_writes | ||
type: scaled_float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long? Same for most fields below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed all fields with float to long.
} | ||
|
||
/* | ||
// TODO: Enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be enabled again I assume?
…into couchdb-module
jenkins, test this |
Thanks @berfinsari! I think we can merge this and continue with follow ups, I have created a new issue to keep track of development of this module (#10354). |
|
||
def get_hosts(self): | ||
return [os.getenv('COUCHDB_HOST', 'localhost') + ':' + | ||
os.getenv('COUCHDB_PORT', '5894')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.getenv('COUCHDB_PORT', '5894')] | |
os.getenv('COUCHDB_PORT', '5984')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fixed in follow up #10358
Merged, @berfinsari do you happen to have some dashboards you'd like to share too? 🙂 |
No description provided.