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.

Sign up for GitHub

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

Jump to bottom

Metricbeat test modules #4656

Merged
merged 7 commits into from
Jul 21, 2017
Merged

Metricbeat test modules #4656

merged 7 commits into from
Jul 21, 2017

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Jul 12, 2017 •
edited
Loading

Add metricbeat test modules command, it goes over all configured metricsets and runs them, reporting the result back to the user:

X1 :: elastic/beats/metricbeat ‹test-modules*› » ./metricbeat test modules        
system...
  cpu...OK
    result: 
    {
     "cores": 4,
     "idle": {
      "pct": 0
     },
     "iowait": {
      "pct": 0
     },
     "irq": {
      "pct": 0
     },
     "nice": {
      "pct": 0
     },
     "softirq": {
      "pct": 0
     },
     "steal": {
      "pct": 0
     },
     "system": {
      "pct": 0
     },
     "user": {
      "pct": 0
     }
    }

nginx...
  stubstatus...
    error... ERROR error making http request: Get http://127.0.0.1/server-status: dial tcp 127.0.0.1:80: getsockopt: connection refused

Filtering by module and metricset:

X1 :: elastic/beats/metricbeat ‹test-modules*› » ./metricbeat test modules system process_summary
system...
  process_summary...OK
    result: 
    {
     "_namespace": "process.summary",
     "idle": 0,
     "running": 1,
     "sleeping": 236,
     "stopped": 0,
     "total": 238,
     "unknown": 1,
     "zombie": 0
    }

X1 :: elastic/beats/metricbeat ‹test-modules*› » ./metricbeat test modules nginx                 
nginx...
  stubstatus...
    error... ERROR error making http request: Get http://127.0.0.1/server-status: dial tcp 127.0.0.1:80: getsockopt: connection refused

TODO:

  • Make it work for dynamic modules
  • Tests

@exekias exekias changed the title [WIP] Metricbeat Test modules [WIP] Metricbeat test modules Jul 12, 2017
@exekias exekias added enhancement Metricbeat Metricbeat needs_docs in progress Pull request is currently in progress. labels Jul 12, 2017
@exekias
Copy link
Contributor Author

exekias commented Jul 13, 2017

jenkins retest this

@exekias exekias force-pushed the test-modules branch 3 times, most recently from 36c49e6 to 4838798 Compare July 17, 2017 14:42
@exekias exekias added review and removed in progress Pull request is currently in progress. labels Jul 17, 2017
exekias added 2 commits July 17, 2017 16:55
@exekias
Add test modules command
3cd4113 
It goes over all configured modules and runs them, reporting the
resulting event to the user
@exekias
Update CHANGELOG
9841538
@exekias exekias changed the title [WIP] Metricbeat test modules Metricbeat test modules Jul 17, 2017
@exekias
Copy link
Contributor Author

exekias commented Jul 17, 2017

jenkins retest this please

andrewkroh
andrewkroh approved these changes Jul 18, 2017
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM, I just left a few questions.

metricbeat/mb/module/wrapper.go
@@ -185,6 +186,11 @@ func (mw *Wrapper) Hash() uint64 {
return mw.configHash
}

// MetricSets return the list of metricsets of the module
func (mw *Wrapper) MetricSets() []*metricSetWrapper {
Copy link
Member

Choose a reason for hiding this comment

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

Can this return []mb.MetricSet instead? (Returning an unexported type from an exported method can be annoying to users.)

Copy link
Contributor Author

@exekias exekias Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That was my first though too, but then I would need to reimplement fetch, which is not a big deal but could cause issues when we add new metricset types, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Then that seems OK.

metricbeat/mb/module/wrapper.go Outdated
// Reporter implementation

type reporter interface {
StartFetchTimer()
Done() <-chan struct{}
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to embed mb.PushReporter in reporter rather than duplicating the method signatures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally, thanks for the tip :)

@exekias
Simplify reporter interface
d273886 
by embedding `mb.PushReporter` in it
@exekias
Copy link
Contributor Author

exekias commented Jul 20, 2017

fixed conflicts with master, all tests passing now :)

tsg
tsg reviewed Jul 21, 2017
View reviewed changes
metricbeat/cmd/test/modules.go

config, err := b.BeatConfig()
if err != nil {
fmt.Fprintf(os.Stderr, "Error initializing beat: %s\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

The three "Error initializing beat" message above might get confusing at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I merged the PR, this is minor and can be handled in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, as I have been adding new commands I feel the need for a common cmdline util to handle initialization and output

@tsg tsg merged commit c1bd89b into elastic:master Jul 21, 2017
@dedemorton dedemorton mentioned this pull request Jul 25, 2017
Closed
42 tasks
@dedemorton
Copy link
Contributor

Removing needs_docs label because the command is documented here: https://www.elastic.co/guide/en/beats/metricbeat/master/command-line-options.html#test-command

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

@tsg tsg tsg left review comments

Assignees
No one assigned
Labels
enhancement Metricbeat Metricbeat review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@exekias @dedemorton @tsg @andrewkroh