-
Notifications
You must be signed in to change notification settings - Fork 2
OOP refactor #4
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
OOP refactor #4
Conversation
- implement a OOP structure with MetricProvider objects that return prometheus Metrics from the json log result - implement an agnostic NvmeCollector that stores MetricProviders instances and delegates handling the details of Collect and Describe to them - move NvmeCollector instantiation in a convenience function in the main package
|
@dobbi84 questa la PR |
55af85e to
9ec9ca9
Compare
| return fmt.Errorf("NVMe cli version not supported, supported versions are: %v", _supportedVersions) | ||
| } | ||
|
|
||
| shell := utils.NewShell( |
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.
Preferirei rimanere su exec.Command ed evitare shell quando possibile
| } | ||
|
|
||
| return &pkg.NvmeCollector{ | ||
| OcpEnabled: ocpEnabled, |
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.
non dovremmo internalizzare OcpEnabled in OcpMetricProvider?
| ch <- logProvider.Desc | ||
| } | ||
|
|
||
| for _, ocpLogProvider := range c.OcpLogMetricProviders { |
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.
dovremmo eseguirlo solo se ocp flag e' settata
| // getDevices queries the devices list through the shell | ||
| // and returns an array of JSON results with the devices data. | ||
| func (c *NvmeCollector) getDevices() []gjson.Result { | ||
| shell := utils.NewShell(utils.WithValidators(gjson.Valid)) |
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.
anche qua preferirei mantenere command invece che shell
| // sendOcpSmartLogMetrics queries the shell for ocp smart-log data, | ||
| // gets the metrics from each OcpLogMetricProvider in c.OcpLogMetricProviders | ||
| // and sends them through the channel. | ||
| func (c *NvmeCollector) sendOcpSmartLogMetrics(ch chan<- prometheus.Metric, device gjson.Result) { |
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.
sembra molto codice duplicato con sendSMartLogMetrics
|
Chiudo per duplicazione con nuovo push su ftalpo/oop_refactor nel main repo |
In questo branch ho fatto un refactor della logica, per renderla "ad oggetti" (tra molte virgolette).
Ora il codice di NvmeCollector, definito in
/pkg/collector.goè agnostico rispetto ai dettagli di tutte le metriche definite.Per ogni metrica viene istanziato un oggetto MetricProvider (
InfoMetricProvideroLogMetricProvider, definiti in/pkg/provider.go), a cui viene delegata l'estrazione del valore dai JSON e la costruzione della corrispettivaprometheus.Metric.In questo modo il collector non deve sapere nulla dei dettagli di ciascuna metrica: i metodi
CollecteDescribeora iterano sulle liste deiMetricProvider, nel caso diCollectdelegando a questi ultimi il fetching della metrica dai log JSON.L'oggetto
NvmeCollectorusato inmain.goora viene istanziato in/cmd/collector.go.Vantaggi: il codice "core" sotto
/pkgnon deve sapere nulla delle implementazioni di ogni metrica, quindi per aggiungere/rimuovere una metrica basterà editare/cmd/collector.go, senza toccare le definizioni dei metodi diNvmeCollector. Tutte le info per la definizione delle metriche sono centralizzate in/cmd/collector.go(nome, help string, JSON key)Svantaggi: il codice è meno lineare, e bisogna saltare tra 2-3 file per capire cosa succede.