Add namespaces to monitoring registry #6836

ruflin · 2018-04-12T11:28:12Z

So far for metricsets and modules only combined metrics were collected. There was no visibility into a single metricset. This PR provides the base functionality to use have each metricset provide it's own metrics.

The BaseMetricSet was extended with an ID() and Metrics() method. The id is required to have a unique identifier for each metricset during reporting and the Metrics() method can be used inside each Mmetricsetetricset to access the metricset specific registry.

Currently the data collected from all metricsets are exposed under /dataset. This is probably going to change and is mainly to show case the first implementation. The data exposed looks as following:

{
  "05d4bd84-2ca2-4d9d-862d-c7822b4389f5": {
    "id": "05d4bd84-2ca2-4d9d-862d-c7822b4389f5",
    "metricset": "process_summary",
    "module": "system",
    "starttime": "2018-04-17 15:41:34.154435426 +0200 CEST m=+10.037457766"
  },
  "2f85ed18-343b-4c33-865c-44e624cdaf6d": {
    "id": "2f85ed18-343b-4c33-865c-44e624cdaf6d",
    "metricset": "load",
    "module": "system",
    "starttime": "2018-04-17 15:41:34.15164335 +0200 CEST m=+10.034665690"
  }
}

The initial idea was to expose an array instead of map with unique keys. But having a map with unique keys makes it easier to add and remove registries when a metricset runner is started / stopped and the functionality already existed. In case the above metrics were reported to Elasticsearch, each block would be one event. To automate this conversion, we could use a flag on a registry to potential have a different reporting for Elasticsearch.

The concept of namespaces was added to manage the existing registries instead of just having on global (Default). This can be used in the future for additional API endpoints.

houndci-bot · 2018-04-12T11:28:22Z

libbeat/monitoring/namespace.go

+	n.registry = r
+}
+
+func (n *Namespace) GetRegistry() *Registry {


exported method Namespace.GetRegistry should have comment or be unexported

houndci-bot · 2018-04-12T11:28:22Z

libbeat/monitoring/namespace.go

+	return newNamespace(name)
+}
+
+func (n *Namespace) SetRegistry(r *Registry) {


exported method Namespace.SetRegistry should have comment or be unexported

houndci-bot · 2018-04-12T11:28:22Z

libbeat/monitoring/namespace.go

+	return n
+}
+
+func GetNamespace(name string) *Namespace {


exported function GetNamespace should have comment or be unexported

houndci-bot · 2018-04-12T11:28:22Z

libbeat/monitoring/namespace.go

+
+var namespaces = map[string]*Namespace{}
+
+type Namespace struct {


exported type Namespace should have comment or be unexported

urso · 2018-04-12T12:02:22Z

Yep, that's basically the idea. Also missing:

Allow for namespaces to have a sub-namespace
Support for iterating namespaces and registries within namespaces
have ES monitoring reporter make use of namespaces and report all registries that that are marked as 'exported'.

ruflin · 2018-04-17T13:56:17Z

@urso I changed my initial approach with namespace quite a bit and also updated the PR message. I realised most of the Add/Remove functionality is already given by the Registry if we use unique id's. The namespace are now mainly a global "registry" for the Registry.

The naming is still super confusing. I think we need to separate monitoring and metrics into 2 packages. This should make things less confusing.

andrewkroh

I like the idea. It fixes a problem we have where metrics from two instances of a metricset are combined. I didn’t look too close at the monitoring API changes.

Adding the beat metadata has been on my todo list. I’d also like to get some of the OS metadata in there too.

andrewkroh · 2018-04-17T23:56:05Z

metricbeat/mb/module/wrapper.go

@@ -106,9 +106,17 @@ func (mw *Wrapper) Start(done <-chan struct{}) <-chan beat.Event {
 	wg.Add(len(mw.metricSets))
 	for _, msw := range mw.metricSets {
 		go func(msw *metricSetWrapper) {
+			metricsPath := fmt.Sprintf("%s", msw.ID())


Why the Sprintf?

This is a left over as I used longer metric paths initially. Will clean it up.

andrewkroh · 2018-04-17T23:58:23Z

metricbeat/mb/module/wrapper.go

 			defer releaseStats(msw.stats)
 			defer wg.Done()
 			defer msw.close()
+
+			registry.Add(metricsPath, msw.Metrics(), monitoring.Full)
+			monitoring.NewString(msw.Metrics(), "starttime").Set(time.Now().String())


Does monitoring have support for dates? If not I’d use common.Time so that the String format is consistent.

There is no format for dates yet but I think it would be valuable to have. Will change it to common.Time for now

andrewkroh · 2018-04-17T23:59:12Z

metricbeat/mb/module/wrapper.go

@@ -106,9 +106,17 @@ func (mw *Wrapper) Start(done <-chan struct{}) <-chan beat.Event {
 	wg.Add(len(mw.metricSets))
 	for _, msw := range mw.metricSets {
 		go func(msw *metricSetWrapper) {
+			metricsPath := fmt.Sprintf("%s", msw.ID())
+			registry := monitoring.GetNamespace("dataset").GetRegistry()


With this we can remove the code associated with getMetricSetStats.

Long term this will the case. I didn't change it in this PR as I didn't want to change any of the existing stats outputs as it could break central monitoring.

andrewkroh · 2018-04-18T00:01:44Z

metricbeat/mb/builders.go

@@ -156,10 +159,22 @@ func newBaseMetricSets(r *Register, m Module) ([]BaseMetricSet, error) {
 	for _, name := range metricSetNames {
 		name = strings.ToLower(name)
 		for _, host := range hosts {
+


Remove this newline?

of course ;-)

andrewkroh · 2018-04-18T00:04:35Z

metricbeat/mb/mb.go

@@ -78,13 +79,15 @@ func (m *BaseModule) UnpackConfig(to interface{}) error {
 // addition to this interface, all MetricSets must implement either
 // EventFetcher or EventsFetcher (but not both).
 type MetricSet interface {
+	ID() string     // Unique id identifying a running MetricSet.


andrewkroh · 2018-04-18T00:05:29Z

metricbeat/mb/mb.go

 	Name() string   // Name returns the name of the MetricSet.
 	Module() Module // Module returns the parent Module for the MetricSet.
 	Host() string   // Host returns a hostname or other module specific value
 	// that identifies a specific host or service instance from which to collect
 	// metrics.
 	HostData() HostData                  // HostData returns the parsed host data.
 	Registration() MetricSetRegistration // Params used in registration.
+	Metrics() *monitoring.Registry       // Metricset specific metrics


s/Metricset/MetricSet/ and a period.

andrewkroh · 2018-04-18T00:09:13Z

metricbeat/mb/module/wrapper.go

 			defer releaseStats(msw.stats)
 			defer wg.Done()
 			defer msw.close()
+
+			registry.Add(metricsPath, msw.Metrics(), monitoring.Full)


It seems redundant to put the metrics under the ID key AND to have an id field in the object.

I'm going back and forth on this one. My initial plan was to show an array instead of having a list with id as key. The nice part with the id in the key is that it works out of the box with our current implementation. The disadvantage is that the data needs to be sent in a different format to elasticsearch (1 event per entry). And that is where the id also in the object is coming from. It makes parsing each object much easier. Otherwise additional logic is needed to get it out of the key.

So far for metricsets and modules only combined metrics were collected. There was no visibility into a single metricset. This PR provides the base functionality to use have each metricset provide it's own metrics. The BaseMetricSet was extended with an `ID()` and `Metrics()` method. The id is required to have a unique identifier for each metricset during reporting and the `Metrics()` method can be used inside each Mmetricsetetricset to access the metricset specific registry. Currently the data collected from all metricsets are exposed under `/dataset`. This is probably going to change and is mainly to show case the first implementation. The data exposed looks as following: ``` { "05d4bd84-2ca2-4d9d-862d-c7822b4389f5": { "id": "05d4bd84-2ca2-4d9d-862d-c7822b4389f5", "metricset": "process_summary", "module": "system", "starttime": "2018-04-17 15:41:34.154435426 +0200 CEST m=+10.037457766" }, "2f85ed18-343b-4c33-865c-44e624cdaf6d": { "id": "2f85ed18-343b-4c33-865c-44e624cdaf6d", "metricset": "load", "module": "system", "starttime": "2018-04-17 15:41:34.15164335 +0200 CEST m=+10.034665690" } } ``` The initial idea was to expose an array instead of map with unique keys. But having a map with unique keys makes it easier to add and remove registries when a metricset runner is started / stopped and the functionality already existed. In case the above metrics were reported to Elasticsearch, each block would be one event. To automate this conversion, we could use a flag on a registry to potential have a different reporting for Elasticsearch. The concept of namespaces was added to manage the existing registries instead of just having on global (`Default`). This can be used in the future for additional API endpoints.

ruflin · 2018-05-04T12:31:32Z

@andrewkroh Could you have an other look. I would like to get this in mainly for starting to use namespaces. The format on the metrics reported will still change as I have to figure out how to modify the ES reporter to report one event for each entry in the dataset but still use some global data. We normally to this magic in Metricsets but would like to have an automated way here.

andrewkroh

LGTM.

I think we could have a meta issue around this change since there are multiple moving pieces here like updating the monitoring output and coming back to metricbeat to remove the old metricset stats.

ruflin · 2018-05-07T08:05:23Z

I opened a meta issue here to track the changes: #7027

ruflin added in progress Pull request is currently in progress. libbeat labels Apr 12, 2018

houndci-bot reviewed Apr 12, 2018

View reviewed changes

ruflin force-pushed the monitoring-namespace branch from 0386233 to d0b9c63 Compare April 17, 2018 13:52

ruflin force-pushed the monitoring-namespace branch from d0b9c63 to 930b2f0 Compare April 17, 2018 13:58

andrewkroh reviewed Apr 18, 2018

View reviewed changes

ruflin force-pushed the monitoring-namespace branch from 930b2f0 to 817f979 Compare April 27, 2018 08:59

ruflin force-pushed the monitoring-namespace branch from 817f979 to e4e5a0d Compare May 4, 2018 12:07

ruflin changed the title ~~[WIP] Add namespaces to monitoring registry~~ Add namespaces to monitoring registry May 4, 2018

ruflin added review and removed in progress Pull request is currently in progress. labels May 4, 2018

andrewkroh approved these changes May 4, 2018

View reviewed changes

andrewkroh merged commit bc4b07c into elastic:master May 4, 2018

ruflin deleted the monitoring-namespace branch May 7, 2018 08:00

ruflin mentioned this pull request May 7, 2018

Improve metric collection and reporting #7027

Closed

5 tasks

andrewkroh mentioned this pull request May 16, 2021

Report dataset metrics in monitoring log reporter #25727

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespaces to monitoring registry #6836

Add namespaces to monitoring registry #6836

ruflin commented Apr 12, 2018 •

edited

Loading

houndci-bot Apr 12, 2018

houndci-bot Apr 12, 2018

houndci-bot Apr 12, 2018

houndci-bot Apr 12, 2018

urso commented Apr 12, 2018

ruflin commented Apr 17, 2018

andrewkroh left a comment

andrewkroh Apr 17, 2018

ruflin Apr 18, 2018

andrewkroh Apr 17, 2018

ruflin Apr 18, 2018

andrewkroh Apr 17, 2018

ruflin Apr 18, 2018

andrewkroh Apr 18, 2018

ruflin Apr 18, 2018

andrewkroh Apr 18, 2018

andrewkroh Apr 18, 2018

andrewkroh Apr 18, 2018

ruflin Apr 18, 2018

ruflin commented May 4, 2018

andrewkroh left a comment

ruflin commented May 7, 2018


		var namespaces = map[string]*Namespace{}

		type Namespace struct {

Add namespaces to monitoring registry #6836

Add namespaces to monitoring registry #6836

Conversation

ruflin commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

urso commented Apr 12, 2018

ruflin commented Apr 17, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented May 4, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

ruflin commented May 7, 2018

ruflin commented Apr 12, 2018 •

edited

Loading