-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add MonitorCustomConfig support to Smart Agent Receiver #54
Conversation
) | ||
|
||
type Config struct { | ||
configmodels.ReceiverSettings `mapstructure:",squash"` | ||
monitorConfig config.MonitorCustomConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
smartagent
receiver 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.