Add MonitorCustomConfig support to Smart Agent Receiver #54
Conversation
rmfitzpatrick
requested review from
pmcollins,
jrcamp,
bogdandrutu,
tigrannajaryan and
asuresh4
| ) | ||
|
|
||
| type Config struct { | ||
| configmodels.ReceiverSettings `mapstructure:",squash"` | ||
| monitorConfig config.MonitorCustomConfig |
There was a problem hiding this comment.
Do I understand it correct that unlike Collector component which declare custom struct for their config all monitors use the same MonitorCustomConfig struct for their configuration?
There was a problem hiding this comment.
In the agent, generally each monitor uses their own unique config type, which embeds a core MonitorConfig struct, or a similarly derived "base": https://github.com/signalfx/signalfx-agent/blob/ffe302c3b3350ae9f94c4e80d57cddf1a2ffe3c2/pkg/monitors/cpu/cpu.go#L31. The MonitorCustomConfig interface implements a method that returns a pointer to this core type (https://github.com/signalfx/signalfx-agent/blob/ce1a2e0a25e75bb6623f29c7c23cd6271ca5defe/pkg/core/config/monitor.go#L249) and the MonitorConfig's pointer receiver method returns itself in the only implementation: https://github.com/signalfx/signalfx-agent/blob/ce1a2e0a25e75bb6623f29c7c23cd6271ca5defe/pkg/core/config/monitor.go#L222
| return fmt.Errorf("you must specify a \"type\" for a smartagent receiver") | ||
| } | ||
|
|
||
| // monitors.ConfigTemplates is a map that all monitors use to register their custom configs in the Smart Agent. |
There was a problem hiding this comment.
Is there any possibility to refactor in order to avoid having a global registry of monitors.ConfigTemplates? We usually try to avoid anything global in the Collector (which can be useful for e.g. creating 2 instances of Collector Service with different sets of factories for testing purposes).
Not necessary to refactor now, but do you see the possibility or it is too complicated?
There was a problem hiding this comment.
This is in the agent, and would require refactoring or an implementation of a plugin system. If I follow you, the factories here are not the same as the Collector component factories: https://github.com/signalfx/signalfx-agent/blob/ce166a6fa14cba30d0559bb87b0f6d4f9cde0784/pkg/monitors/monitor.go#L15. These are functions that* return new monitor instances: https://github.com/signalfx/signalfx-agent/blob/ce166a6fa14cba30d0559bb87b0f6d4f9cde0784/pkg/monitors/cpu/cpu.go#L27
rmfitzpatrick
force-pushed
the
smartagentconfig
branch
from
46afb6b
to
275983f
Compare
|
@jrcamp mind giving this a look w/ your SA expertise when possible? |
These changes address the first remaining item detailed in #53 (comment). In most cases they allow users to c/p SA monitor configs to corresponding
smartagentreceiver config blocks, which will be later used by the receiver to invoke the applicable monitor factories. In cases where config tag* values are maps with case sensitive strings, this implementation does not correctly translate since the Collector config is based on viper, which destroys all record of tag key cases. A translation approach for these cases will be implemented when necessary, though the majority of use cases are not planned to be supported by the Smart Agent Receiver (e.g. in-monitor metric transformations).At a high level this implementation works similarly to the Smart Agent's dynamic config functionality, and is based on the reflection package to create and populate custom monitor config content from the provided yaml. It uses a custom viper unmarshaller and the central SA monitor registration maps to meet this end.
Also including required go modules replace directives to import the agent and a required windows test job environment correction.