Replace pulling metrics port with using direct config #2856
Conversation
|
|
||
| // GetTelemetry serves as a helper function to access the fields we care about in the underlying telemetry struct. | ||
| // This exists to avoid needing to worry extra fields in the telemetry struct. | ||
| func (s *Service) GetTelemetry() *Telemetry { |
There was a problem hiding this comment.
I wonder if we should have a method on Config to convert it to a more constrained and strictly typed struct containing only information we make use of in the operator. Not a request to do it in this PR, just a thought.
On a separate note, should we cache this to avoid the marshalling and unmarshalling whenever it's accessed?
There was a problem hiding this comment.
Yeah that feels like a good follow up to this – both of those are things we should figure out after we simplify all of the methods that use only maps and strings. Caching would get simpler if we only pass around that constrained struct (because we would probably only create it once)
| jsonData, err := json.Marshal(s.Telemetry) | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| t := &Telemetry{} | ||
| // Unmarshal JSON into the provided struct | ||
| if err := json.Unmarshal(jsonData, t); err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do you think we should check this in the admission webhook, so we can be reasonably sure it works? Or is it better to be more permissive and then simply ignore functionality that depends on this value?
There was a problem hiding this comment.
I think its better to be permissive IMO, it's much closer to what we do today (i.e. it's fine if someone doesn't set this)... I think we could maybe introduce a "strict" operator mode as part of this issue's resolution
There was a problem hiding this comment.
I was thinking about the situation where the parsing fails, that is, the format is inconsistent with what we expect. I think, other than a breaking change in the respective collector core package, this would be safe to do.
With that said, breaking changes in collector core packages can happen, and it'd be unfortunate for us to need to always do a breaking change in the CRD as a result.
There was a problem hiding this comment.
Yeah I think once everything in the collector is marked stable we could do this... they're working on it!
…#2856) * checkpoint: with working tests, minimal * chlog * issue * missing change
…#2856) * checkpoint: with working tests, minimal * chlog * issue * missing change
Description:
This PR makes it possible to pull the set self-monitoring port from the config directly as opposed to working off interfaces and strings.
NOTE: I'm trying to minimize changes, because the next one is going to be... large... example
Link to tracking Issue(s):
This is the first part of #2603
Testing:
Unit tests
Documentation:
n/a