Decouple config models from internal models #1161
Conversation
| @@ -347,6 +355,160 @@ func (m *Metric) validateMetric(metricIdx int, parent string, discovery *JobLeve | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *ScrapeConf) toModelConfig() model.JobsConfig { | |||
There was a problem hiding this comment.
Since YACE has the two entry points there is a risk that this mapping diverges from what's in use in exporter.go. IE a field is added to model.JobsConfig but not given a config field/mapped from the config. I can't say I know how to easily test for something like this but just something to note.
There was a problem hiding this comment.
I think I see what you mean. It should be responsibility of config loader to make sure that any field in the config file is mapped correctly/fully to the model.JobsConfig. Likewise, when adding or editing fields in model.JobsConfig one should go back to the config parser to populate those fields correcly (otherwise the changes might be of no use).
pkg/config/config.go
Outdated
| for _, role := range customNamespaceJob.Roles { | ||
| job.Roles = append(job.Roles, model.Role{ | ||
| RoleArn: role.RoleArn, | ||
| ExternalID: role.ExternalID, | ||
| }) | ||
| } | ||
|
|
||
| for _, customTag := range customNamespaceJob.CustomTags { | ||
| job.CustomTags = append(job.CustomTags, model.Tag{ | ||
| Key: customTag.Key, | ||
| Value: customTag.Value, | ||
| }) | ||
| } | ||
|
|
||
| for _, metric := range customNamespaceJob.Metrics { | ||
| job.Metrics = append(job.Metrics, &model.MetricConfig{ | ||
| Name: metric.Name, | ||
| Statistics: metric.Statistics, | ||
| Period: metric.Period, | ||
| Length: metric.Length, | ||
| Delay: metric.Delay, | ||
| NilToZero: metric.NilToZero, | ||
| AddCloudwatchTimestamp: metric.AddCloudwatchTimestamp, | ||
| }) | ||
| } |
There was a problem hiding this comment.
If I'm reading this right it looks like these three mappings show up for each job type. Thoughts on a making a toModelRole, toModelTag, and toModelMetricConfig?
There was a problem hiding this comment.
Good call, done!
| job.ExportedTagsOnMetrics = []string{} | ||
| if len(c.Discovery.ExportedTagsOnMetrics) > 0 { | ||
| svc := SupportedServices.GetService(job.Type) | ||
| if exportedTags, ok := c.Discovery.ExportedTagsOnMetrics[svc.Namespace]; ok { | ||
| job.ExportedTagsOnMetrics = exportedTags | ||
| } else if exportedTags, ok := c.Discovery.ExportedTagsOnMetrics[svc.Alias]; ok { | ||
| job.ExportedTagsOnMetrics = exportedTags | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the code which fixes #1160 right?
I'm a bit surprised that this field exists at the root of discovery (I think you mentioned this elsewhere as well). I guess you could have multiple discovery jobs for the same service but that seems more like an exception vs the normal.
There was a problem hiding this comment.
I guess you could have multiple discovery jobs for the same service but that seems more like an exception vs the normal.
Yes I think that is more of an exception -- that case however should be covered by this transformation loop.
With this change, models used for config parsing are fully decoupled from models used internally at scrape time. Once the config is parsed and validated, it is converted to a representation used internally for the actual scrape logic. The down side of this approach: - structs appear duplicated between the `config` and `models` - after parsing we go through a complete mapping of fields to the internal models representation (this has unnoticeable impact at startup and zero impact at runtime) On the pros side of things: - allows to continue improving the design of internal models without impacting the configuration - allows multiple config format to co-exist Coincidentally this also fixes #1160
cristiangreco
force-pushed
the
cristian/config-models
branch
from
cafebbb to
2caa711
Compare
In a previous PR I've changed the Load() signature to include the conversion of the yaml-based config into internal representation. This PR changes the Validate() function as well so that library users can validate and convert an in-memory config in one pass. Followup of #1161
With this change, models used for config parsing are fully decoupled from models used internally at scrape time. Once the config is parsed and validated, it is converted to a representation used internally for the actual scrape logic.
The down side of this approach:
configandmodelsOn the pros side of things:
Coincidentally this also fixes #1160