Conversation
|
💚 CLA has been signed |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
/test |
2 similar comments
|
/test |
|
/test |
There was a problem hiding this comment.
This description is wrong. The package description should be extracted from this file https://github.com/elastic/beats/blob/master/metricbeat/module/etcd/_meta/docs.asciidoc and then each metricset will have its own [metricset]/_meta/docs.asciidoc file to extract the info for this file from.
There was a problem hiding this comment.
Insert the sample event in this doc too. Look at an example of how to do it here: https://raw.githubusercontent.com/elastic/integrations/master/packages/kibana/_dev/build/docs/README.md
Basically you need to use {{event [metricset]}} or {{fields [metricset]}}
sayden
left a comment
There was a problem hiding this comment.
Left some comments but it's almost finished
packages/etcd/manifest.yml
Outdated
There was a problem hiding this comment.
This should now be "^7.15.1" || ^8.0.0"
|
/test |
1 similar comment
|
/test |
packages/etcd/changelog.yml
Outdated
There was a problem hiding this comment.
nit: you can remove the comment
There was a problem hiding this comment.
I'm wondering if you need to build the Docker image if you're using the default bitnami/etcd. Did you try to go without Dockerfile?
There was a problem hiding this comment.
nit: capital letter (Successful)
There was a problem hiding this comment.
nit: capital letter (Failed)
There was a problem hiding this comment.
If you have to be consistent with letter-casing: ETCD vs etcd vs Etcd. I think the official name is "etcd". Would you mind double-check the rest of the PR.
There was a problem hiding this comment.
This metrics is being read from the Etcd V3 endpoint and won’t show any activity regarding Etcd V2.
I think this information is redundant as you already described it above, before listing data streams. I would recommend to add 1-2 sentences to every data stream and describe what kind of metrics will the data stream proivde.
There was a problem hiding this comment.
We don't use "metricsets" anymore. This is a term valid for Beats only. Integrations are using data streams. Please adjust the entire PR accordingly.
There was a problem hiding this comment.
Is it possible to follow the snake case? For example: append_request, bandwidth_rate
|
/test |
mtojek
left a comment
There was a problem hiding this comment.
Once this PR is merged, do you plan to work on dashboards?
packages/etcd/manifest.yml
Outdated
There was a problem hiding this comment.
Let's not release it as "GA", but "experimental" or "beta".
There was a problem hiding this comment.
Yes, I can work on dashboards.
There was a problem hiding this comment.
Could you please try also against other versions?
mtojek
left a comment
There was a problem hiding this comment.
I re-reviewed the PR and it looks correct. I left one comment regarding definitions of services for tests.
What does this PR do?
Adds etcd metricset.
Needs a dashboard.
Checklist
changelog.ymlfile.manifest.ymlfile to point to the latest Elastic stack release (e.g.^7.13.0).How to test this PR locally