Publisher pipeline: pass logger and metrics registry #8091
Conversation
libbeat/publisher/pipeline/module.go
Outdated
| @@ -38,6 +38,12 @@ var publishDisabled = false | |||
|
|
|||
| const defaultQueueType = "mem" | |||
|
|
|||
| type Monitors struct { | |||
There was a problem hiding this comment.
exported type Monitors should have comment or be unexported
libbeat/publisher/pipeline/module.go
Outdated
| @@ -38,6 +38,12 @@ var publishDisabled = false | |||
|
|
|||
| const defaultQueueType = "mem" | |||
|
|
|||
| type Monitors struct { | |||
| Metrics *monitoring.Registry | |||
| Telemetry *monitoring.Registry | |||
There was a problem hiding this comment.
Interesting separate of telemetry and metrics here. At the moment the separation is pretty clear but I don't think it will stay like this. Perhaps we better pass a map[string]*monitoring.Registry instead?
There was a problem hiding this comment.
I prefer the proposed way instead of map[string]*monitoring.Registry, using a field reflect a clear intends, using a map complexify the access patterns instead of a nil check.
I would argue that maybe the monitoring package could expose a struct to list the possible monitors so we only have one place to change and the access definition are centralized.
Concerning having Logger inside the monitor struct Monitor, at first I was against a logger is effectively some kind of monitor.
There was a problem hiding this comment.
I agree with PH, using a map will complicate the code and make it much more error prone. Instead we should have well defined types. Right now we use globals in a few places, so I kept the struct local to the publisher pipeline for now (makes me wonder if I missed some globals in the outputs).
Wasn't sure about the logger, but then decided to put it into the struct as well. Ideas was: all functionality passed/configured is for the purpose of getting some insights into an instance of the publisher pipeline. The metrics registry share the same type, but they are used in very different ways as well.
libbeat/publisher/pipeline/module.go
Outdated
| @@ -38,6 +38,12 @@ var publishDisabled = false | |||
|
|
|||
| const defaultQueueType = "mem" | |||
|
|
|||
| type Monitors struct { | |||
| Metrics *monitoring.Registry | |||
| Telemetry *monitoring.Registry | |||
There was a problem hiding this comment.
I prefer the proposed way instead of map[string]*monitoring.Registry, using a field reflect a clear intends, using a map complexify the access patterns instead of a nil check.
I would argue that maybe the monitoring package could expose a struct to list the possible monitors so we only have one place to change and the access definition are centralized.
Concerning having Logger inside the monitor struct Monitor, at first I was against a logger is effectively some kind of monitor.
|
@urso |
Yeah, that's why PR is still in progress. There might be some more test builds I have to fix before PR goes green. |
There was a problem hiding this comment.
LGTM
@ph Do you want to have an other look?
@urso We should make sure that the queue types show potentially show up in monitoring and telemetry which requires a few changes in ES and KB. Do you want to open an issue in https://github.com/elastic/stack-monitoring ?
|
@urso LGTM but lets add a simple changelog |
We should strive to not have dependencies on globals in beats. The publisher pipeline rewrite made sure we don't work with globals internally. Yet some globals have been introduced since, and even though the library didn't use globals internally, initialization still did use globals at some points. This change removes globals for logging/metrics/telemetry, by requiring the beat instance to pass down required instances.
155f723 to
ade2188
Compare
* includes minor updates for elastic/beats#8091 NewBroker change
* includes minor updates for elastic/beats#8091 NewBroker change
We should strive to not have dependencies on globals in beats. The publisher pipeline rewrite made sure we don't work with globals internally. Yet some globals have been introduced since, and even though the library didn't use globals internally, initialization still did use globals at some points. This change removes globals for logging/metrics/telemetry, by requiring the beat instance to pass down required instances. (cherry picked from commit 8c15e6e)
We should strive to not have dependencies on globals in beats. The publisher pipeline rewrite made sure we don't work with globals internally. Yet some globals have been introduced since, and even though the library didn't use globals internally, initialization still did use globals at some points. This change removes globals for logging/metrics/telemetry, by requiring the beat instance to pass down required instances. (cherry picked from commit 8c15e6e)
We should strive to not have dependencies on globals in beats. The
publisher pipeline rewrite made sure we don't work with globals
internally. Yet some globals have been introduced since, and even though
the library didn't use globals internally, initialization still did use
globals at some points.
This change removes globals for logging/metrics/telemetry, by requiring
the beat instance to pass down required instances.