NETOBSERV-1427: Added config watcher and supporting metrics reloading #607
Conversation
|
@OlivierCazade: This pull request references NETOBSERV-1427 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
pkg/pipeline/encode/encode_prom.go
Outdated
| } | ||
| } | ||
|
|
||
| func (e *EncodeProm) cleanDeletedCounters(newCfg api.PromEncode) { |
There was a problem hiding this comment.
after rebasing this code can probably be simplified, with all cleanup functions merged in a single one, as Kalman did use a common struct for all the metric types
pkg/pipeline/encode/encode_prom.go
Outdated
| switch mCfg.Type { | ||
| case api.MetricEncodeOperationName("Counter"): | ||
| if oldMetric, ok := e.counters[fullMetricName]; ok { | ||
| if !reflect.DeepEqual(mInfo, oldMetric.info) { |
There was a problem hiding this comment.
I'm not sure if that works as intended, as info contains a predicate (a function) so they'll likely never be equal: https://pkg.go.dev/reflect#DeepEqual
You could test just the api definition oldMetric.info.MetricsItem
pkg/pipeline/pipeline_watcher.go
Outdated
| func (pcw *pipelineConfigWatcher) updateFromConfigmap(cm *corev1.ConfigMap) { | ||
| if rawConfig, ok := cm.Data[pcw.configFile]; ok { | ||
| config := config.ConfigHotReloadStruct{} | ||
| err := json.Unmarshal([]byte(rawConfig), config) |
There was a problem hiding this comment.
govet messages below seem relevant, I guess Unmarshalling would fail here
pkg/pipeline/encode/encode_prom.go
Outdated
|
|
||
| } | ||
|
|
||
| func (e *EncodeProm) cleandDeletedMetrics(newCfg api.PromEncode) { |
There was a problem hiding this comment.
Perhaps cleanDeletedMetrics (without the extra d)?
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
7f972e1 to
60388fb
Compare
OlivierCazade
force-pushed
the
1427-hotreload
branch
3 times, most recently
from
cc471df to
3d16544
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 67.41% 65.11% -2.31%
==========================================
Files 102 112 +10
Lines 7589 7940 +351
==========================================
+ Hits 5116 5170 +54
- Misses 2162 2456 +294
- Partials 311 314 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
3d16544 to
943bc9f
Compare
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
943bc9f to
4750c83
Compare
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
4750c83 to
2e05954
Compare
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
2e05954 to
387a8ce
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 67.43% 66.49% -0.94%
==========================================
Files 102 104 +2
Lines 7593 6659 -934
==========================================
- Hits 5120 4428 -692
+ Misses 2162 1916 -246
- Partials 311 315 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
OlivierCazade
force-pushed
the
1427-hotreload
branch
3 times, most recently
from
6564c33 to
69fe1b7
Compare
OlivierCazade
force-pushed
the
1427-hotreload
branch
4 times, most recently
from
ca4a86c to
64d701b
Compare
|
/retest |
OlivierCazade
changed the title
OlivierCazade
force-pushed
the
1427-hotreload
branch
from
64d701b to
c306f62
Compare
|
thanks @OlivierCazade ! |
|
/label qe-approved |
|
@OlivierCazade: This pull request references NETOBSERV-1427 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Added new type to watch a configmap and notify prometheus encoder
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.