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 couchdb module #9406

Merged
merged 25 commits into from Jan 28, 2019

Conversation

Projects
None yet
6 participants
@berfinsari
Copy link
Contributor

commented Dec 5, 2018

No description provided.

berfinsari added some commits Dec 5, 2018

@elasticmachine

This comment has been minimized.

Copy link

commented Dec 5, 2018

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?

@elasticmachine

This comment has been minimized.

Copy link

commented Dec 6, 2018

@sayden sayden self-assigned this Dec 6, 2018

@berfinsari

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

Is there anything that I can do for this unsuccessful check?

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2018

@berfinsari Try to run make update and make fmt and then push again. This will generate / update a few files.

@berfinsari

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

I ran make update and make fmt commands and nothing changes. Is there anything else I can do?

@jsoriano

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

@berfinsari you need to run make update fmt from the beats directory so it is applied to all beats. In this case the outdated file is in x-pack/metricbeat/, you can also run make update fmt from this directory.

@berfinsari berfinsari requested a review from elastic/beats-contributors as a code owner Dec 14, 2018

@jsoriano
Copy link
Member

left a comment

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.

@jsoriano

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

@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?

Show resolved Hide resolved metricbeat/module/couchdb/_meta/test/serverstats.json Outdated
"httpd_status_codes": common.MapStr{
"200": common.MapStr{
"description": data.HttpdStatusCodes.Num200.Description,
"current": data.HttpdStatusCodes.Num200.Current,

This comment has been minimized.

Copy link
@ruflin

ruflin Dec 17, 2018

Collaborator

What exactly means current in this context? Since the last request?

This comment has been minimized.

Copy link
@berfinsari

berfinsari Dec 19, 2018

Author Contributor

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.

berfinsari added some commits Dec 19, 2018

@berfinsari

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

@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.

@jsoriano

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@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!

@sayden

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

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

This comment has been minimized.

Copy link
@jsoriano

jsoriano Dec 20, 2018

Member

I think fields here should be named just with the number i.e. 200 instead of num200.

@sayden sayden self-requested a review Dec 20, 2018

@berfinsari berfinsari requested a review from elastic/infrastructure as a code owner Jan 8, 2019

@urso urso removed the request for review from elastic/beats Jan 8, 2019

Show resolved Hide resolved metricbeat/module/couchdb/_meta/fields.yml
Show resolved Hide resolved metricbeat/module/couchdb/server/_meta/fields.yml

// New creates a new instance of the MetricSet.
func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
cfgwarn.Experimental("The couchdb server metricset is experimental.")

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 9, 2019

Member

I think we can go directly to beta for these metricsets, please update it in the fields files too.

Show resolved Hide resolved metricbeat/module/couchdb/_meta/docs.asciidoc
"github.com/stretchr/testify/assert"
)

const testFile = "../_meta/test/serverstats.json"

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 9, 2019

Member

Not used variable.

@@ -0,0 +1,43 @@
"couchdb": {

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 9, 2019

Member

Is this an incomplete JSON? It should start by {. The error in travis seems caused by this.

This comment has been minimized.

Copy link
@berfinsari

berfinsari Jan 10, 2019

Author Contributor

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)

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 9, 2019

Member

Check for errors here.

This comment has been minimized.

Copy link
@berfinsari

berfinsari Jan 10, 2019

Author Contributor

Thanks for your review. I believe I've addressed all the feedback now.

berfinsari added some commits Jan 9, 2019

@@ -0,0 +1,58 @@
{
"@timestamp":"2016-05-23T08:05:34.853Z",

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 10, 2019

Collaborator

As the indentation is off in this file and contains some old fields, I wonder if this was generated or manually created?

This comment has been minimized.

Copy link
@berfinsari

berfinsari Jan 11, 2019

Author Contributor

This was generated. Should I update this file?

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 14, 2019

Collaborator

Yes, can you generated it again to see if the strange indentation disappears?

Number of HTTP COPY requests
- name: HEAD
type: scaled_float

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 10, 2019

Collaborator

Should these header counters(?) be long instead of float?

HTTP status codes statistics
fields:
- name: "200"
type: scaled_float

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 10, 2019

Collaborator

Same question as above: long instead of float?

couchdb statistics
fields:
- name: database_writes
type: scaled_float

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 10, 2019

Collaborator

long? Same for most fields below.

This comment has been minimized.

Copy link
@berfinsari

berfinsari Jan 11, 2019

Author Contributor

I changed all fields with float to long.

}

/*
// TODO: Enable

This comment has been minimized.

Copy link
@ruflin

ruflin Jan 14, 2019

Collaborator

This should be enabled again I assume?

berfinsari added some commits Jan 26, 2019

@jsoriano

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

jenkins, test this

@jsoriano jsoriano referenced this pull request Jan 27, 2019

Open

CouchDB metribeat module #10354

7 of 13 tasks complete
@jsoriano

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

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')]

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 27, 2019

Member
Suggested change
os.getenv('COUCHDB_PORT', '5894')]
os.getenv('COUCHDB_PORT', '5984')]

This comment has been minimized.

Copy link
@jsoriano

jsoriano Jan 28, 2019

Member

To be fixed in follow up #10358

@jsoriano jsoriano merged commit 6293568 into elastic:master Jan 28, 2019

4 of 5 checks passed

beats-ci Build finished.
Details
CLA Commit author has signed the CLA
Details
Hound No violations found. Woof!
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsoriano

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

Merged, @berfinsari do you happen to have some dashboards you'd like to share too? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.