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

Improve documentation for server metricset in Zookeeper metricbeat module #10692

Merged
merged 3 commits into from
Mar 1, 2019

Conversation

arsalan0c
Copy link
Contributor

@arsalan0c arsalan0c commented Feb 12, 2019

Resolve #10376

Fields to improve:

  • connections
  • avg latency
  • max latency
  • min latency
  • mode
  • node_count
  • outstanding
  • received
  • sent
  • version_date
  • zxid
  • count
  • epoch

@arsalan0c arsalan0c requested a review from a team as a code owner February 12, 2019 12:23
@elasticmachine
Copy link
Collaborator

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?

@ruflin ruflin requested a review from sayden February 12, 2019 15:00
@ruflin ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 12, 2019
@sayden
Copy link
Contributor

sayden commented Feb 12, 2019

Hi @arsalanc-v2 . An absolutely amazing job. Even I'm learning things of Zookeeper I didn't know with this descriptions.

You still need to sign the CLA, read https://github.com/elastic/beats/blob/master/CONTRIBUTING.md to see how. Also, a change in fields.yml requires an update of the docs a few things more. You can achieve this by running make update from Metricbeat folder.

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

make update needs to be run

@arsalan0c arsalan0c requested a review from a team as a code owner February 13, 2019 09:08
@arsalan0c arsalan0c force-pushed the improve_docs branch 3 times, most recently from 121d372 to 7a4d48f Compare February 13, 2019 09:41
@arsalan0c
Copy link
Contributor Author

Thank you! I have signed the CLA, please let me know if there is anything else I need to do to make the check pass.

@sayden
Copy link
Contributor

sayden commented Feb 13, 2019

Left you some comments, it seems you need a rebase ensure that you aren't adding anything unrelated to the branch. Then make collect && make update

@@ -81,7 +81,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- The `elasticsearch/deprecation` fileset now indexes the `component` field under `elasticsearch` instead of `elasticsearch.server`. {pull}10445[10445]
- Remove field `kafka.log.trace.full` from kafka.log fielset. {pull}10398[10398]
- Change field `kafka.log.class` for kafka.log fileset from text to keyword. {pull}10398[10398]
- Address add_kubernetes_metadata processor issue where old source field is
- Address add_kubernetes_metadata processor issue where old source field is
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't go here. It seems some branching conflict. Try to rebase to master again and running make collect && make update

@arsalan0c arsalan0c force-pushed the improve_docs branch 2 times, most recently from 80fefee to bcfc44e Compare February 14, 2019 06:48
@arsalan0c
Copy link
Contributor Author

When running either make collect or make update, an error appears exactly as the one mentioned here #7489 . However, the solution proposed there (manually creating the virtual environment) still results in the same error. It occurs right after "Generated fields.yml for metricbeat to ..."

It seems like a common problem with other projects but I have not found a solution that would be applicable here

@sayden
Copy link
Contributor

sayden commented Feb 14, 2019

Yeah, it's common because our build system is done with Python 2 and it's tricky to make it work if you also have Python 3 installed. Ensure that you have Python 2 installed, of course :)

The workaround there should work, however. Try this way from Metricbeat folder:

VIRTUALENV_PARAMS="-p python2.7" make python-env
source build/python-env/bin/activate
VIRTUALENV_PARAMS="-p python2.7" make collect-docs
VIRTUALENV_PARAMS="-p python2.7" make update

To really force

@arsalan0c
Copy link
Contributor Author

Unfortunately that does not work either.
image

Is it possible that despite the initial Python 2.7 deprecation message, Python 3 is still being used for subsequent processes? Also, would you recommend I uninstall Python 3?

@ruflin
Copy link
Member

ruflin commented Feb 14, 2019

@arsalanc-v2 It's just a guess but I have seen similar things in the past having both python version and some dependencies for Python 2.7 were taken from Python 3 and that created a mess. Uninstalling Python 3 could definitively help and then remove / install the dependencies again. But not sure if that is a bit too much to ask to change your env.

Seeing you are on OS X, do you use brew? Perhaps you could use pyenv to manage your python versions?

@sayden
Copy link
Contributor

sayden commented Feb 15, 2019

Yes, it's definitely using Python 3 in some moment. It has happened to me in the past too.

You probably have a symlink from python command to your python3 binary. You can also try to backup this symlink (I don't know where it is located in OSX but check with whereis python) and then symlink python2 binary to python. I think this is how I solved it

@arsalan0c
Copy link
Contributor Author

@ruflin @sayden Thank you for your help! The commands ran successfully on an ubuntu vm.

Yes I use brew and pyenv seems like a great tool, I'll be using it from now on.

@sayden
Copy link
Contributor

sayden commented Feb 18, 2019

@arsalanc-v2 don't forget that you need to sign the CLA https://www.elastic.co/contributor-agreement/

The PR looks done or almost done. Let's see what Jenkins thinks :)

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

Jenkins is happy. So we are just waiting you to sign the CLA and we are good to go

@arsalan0c
Copy link
Contributor Author

arsalan0c commented Feb 18, 2019

Awesome, I enjoyed working on this. I signed the CLA on 12/02/2019 and received the pdf by email. Is there anything else I need to do to make the check pass?

@jsoriano
Copy link
Member

@arsalanc-v2 please check if you signed the CLA with the same email address as the used for your commits, you can see in https://github.com/elastic/beats/pull/10692.patch the email in the commits.

@arsalan0c arsalan0c force-pushed the improve_docs branch 5 times, most recently from e4a831f to 4f13e58 Compare February 21, 2019 13:39
@arsalan0c
Copy link
Contributor Author

arsalan0c commented Feb 21, 2019

@jsoriano The same email was used but the name was different. I've resolved that and the patch file shows the correct name now (:

@sayden
Copy link
Contributor

sayden commented Feb 21, 2019

jenkins, test this please

@jsoriano
Copy link
Member

@arsalanc-v2 sorry, there was some issue with CLA signing last week, could you try to sign it again? 🤞

@arsalan0c
Copy link
Contributor Author

arsalan0c commented Feb 26, 2019

@jsoriano Done!

@sayden
Copy link
Contributor

sayden commented Feb 26, 2019

Awesome @arsalanc-v2 Let's wait CI to go green before merging :)

@sayden
Copy link
Contributor

sayden commented Feb 27, 2019

Oh, @arsalanc-v2 I didn't realize until now. Forgive me but we don't add CHANGELOG entries to docs updates. Can you remove it so we can merge the PR 🙂 ?

@@ -30,6 +31,8 @@ import (
var Version = "9.9.9"
var Name = "mockbeat"

var Settings = instance.Settings{Name: Name, Version: Version}

Choose a reason for hiding this comment

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

exported var Settings should have comment or be unexported

@@ -24,7 +24,7 @@ import (
"github.com/elastic/beats/libbeat/mock"
)

var RootCmd = cmd.GenRootCmd(mock.Name, mock.Version, mock.New)
var RootCmd = cmd.GenRootCmdWithSettings(mock.New, mock.Settings)

Choose a reason for hiding this comment

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

exported var RootCmd should have comment or be unexported

@@ -29,18 +29,18 @@ import (
"github.com/elastic/beats/libbeat/testing"
)

func GenTestOutputCmd(name, beatVersion string) *cobra.Command {
func GenTestOutputCmd(settings instance.Settings) *cobra.Command {

Choose a reason for hiding this comment

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

exported function GenTestOutputCmd should have comment or be unexported

@@ -27,18 +27,18 @@ import (
"github.com/elastic/beats/libbeat/cmd/instance"
)

func GenTestConfigCmd(name, version string, beatCreator beat.Creator) *cobra.Command {
func GenTestConfigCmd(settings instance.Settings, beatCreator beat.Creator) *cobra.Command {

Choose a reason for hiding this comment

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

exported function GenTestConfigCmd should have comment or be unexported

@@ -31,7 +31,7 @@ import (
"github.com/elastic/beats/libbeat/template"
)

func GenTemplateConfigCmd(settings instance.Settings, name, idxPrefix, beatVersion string) *cobra.Command {
func GenTemplateConfigCmd(settings instance.Settings) *cobra.Command {

Choose a reason for hiding this comment

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

exported function GenTemplateConfigCmd should have comment or be unexported

@@ -44,3 +44,4 @@ func (p Pipe) Name() string { return p.File.Name() }
func (p Pipe) Stat() (os.FileInfo, error) { return p.File.Stat() }
func (p Pipe) Continuable() bool { return false }
func (p Pipe) HasState() bool { return false }
func (p Pipe) Removed() bool { return false }

Choose a reason for hiding this comment

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

exported method Pipe.Removed should have comment or be unexported


type File struct {
*os.File
}

func (File) Continuable() bool { return true }
func (File) HasState() bool { return true }
func (f File) Removed() bool { return file.IsRemoved(f.File) }

Choose a reason for hiding this comment

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

exported method File.Removed should have comment or be unexported

@arsalan0c
Copy link
Contributor Author

@sayden Ah I see, no worries I've removed it. I read that from the contributing guide

@arsalan0c
Copy link
Contributor Author

Also, I mistakenly pushed some old code (resolved now)

@sayden sayden removed request for a team February 28, 2019 11:53
@sayden
Copy link
Contributor

sayden commented Feb 28, 2019

jenkins, test this

@sayden sayden merged commit 90f1f1a into elastic:master Mar 1, 2019
@sayden
Copy link
Contributor

sayden commented Mar 1, 2019

Good work @arsalanc-v2 👍

@ruflin
Copy link
Member

ruflin commented Mar 4, 2019

@sayden Should we backport this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve documentation 'server' Metricset in Zookeeper Metricbeat module
7 participants