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

Adding query APIs for metricsets and modules from metricbeat registry #4102

Merged
merged 2 commits into from Apr 25, 2017

Conversation

Projects
None yet
5 participants
@vjsamuel
Contributor

vjsamuel commented Apr 24, 2017

No description provided.

@elasticmachine

This comment has been minimized.

elasticmachine commented Apr 24, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine

This comment has been minimized.

elasticmachine commented Apr 24, 2017

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

@ruflin

ruflin approved these changes Apr 25, 2017

@@ -6,6 +6,7 @@ import (
"strings"
"github.com/elastic/beats/libbeat/logp"
"sync"

This comment has been minimized.

@ruflin

ruflin Apr 25, 2017

Collaborator

this should be with strings

@ruflin ruflin requested a review from andrewkroh Apr 25, 2017

r.RLock()
defer r.RUnlock()
modules := []string{}

This comment has been minimized.

@andrewkroh

andrewkroh Apr 25, 2017

Member

Change this to make([]string, 0, len(r.modules)) since it knows the capacity requirement a priori.

r.RLock()
defer r.RUnlock()
metricsets := []string{}

This comment has been minimized.

@andrewkroh
sets, ok := r.metricSets[module]
if ok {
for name := range sets {

This comment has been minimized.

@andrewkroh

andrewkroh Apr 25, 2017

Member

Once it reaches this point, it can allocate the slice with the metricsets = make([]string, 0, len(sets).

@@ -46,6 +47,8 @@ type metricSetFactoryInfo struct {
// Register contains the factory functions for creating new Modules and new
// MetricSets.
type Register struct {
//Lock to control concurrent read/writes

This comment has been minimized.

@andrewkroh

andrewkroh Apr 25, 2017

Member

This comment needs a space after the //.

@@ -46,6 +47,8 @@ type metricSetFactoryInfo struct {
// Register contains the factory functions for creating new Modules and new
// MetricSets.
type Register struct {
//Lock to control concurrent read/writes
sync.RWMutex

This comment has been minimized.

@andrewkroh

andrewkroh Apr 25, 2017

Member

I think embedding the mutex is not a good choice here because the methods will be become part of Register's public API. Users should never need to lock/unlock the Register because that is handled internally.

This reminds me, can you add to the godoc for Register that "It is safe for concurrent usage."`.

@andrewkroh

This comment has been minimized.

Member

andrewkroh commented Apr 25, 2017

jenkins, test it

@andrewkroh andrewkroh merged commit 354fdd6 into elastic:master Apr 25, 2017

6 checks passed

CLA Commit author has signed the CLA
Details
codecov/patch 100% of diff hit (target 64.64%)
Details
codecov/project 64.69% (+0.04%) compared to 2bc0a20
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@vjsamuel vjsamuel deleted the vjsamuel:add_metricsets_registry_query branch May 2, 2017

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Adding query APIs for metricsets and modules from metricbeat registry (
…elastic#4102)

* Adding query APIs for metricsets and modules from metricbeat registry
* Added locking to make `mb.Register` thread-safe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment