Handle backwards compatibility of internal ballast removal #759
Conversation
This change adds a `ballastParserProvider` which wraps a `ConfigSourceParserProvider` and adds a `memory_ballast` extension to the Otel config if one isn't already present. Doing so ensures that users have memory ballast regardless of whether they explicitly added the newly-developed `memory_ballast` extension, which may not have existed when they set up their collector.
|
I think that the main issue to address is what happens if the deprecated ballast field will get removed from the memory limiter processor |
So, to my knowledge, the ballast field in the memory limiter processor never actually allocated a ballast. The memory limiter just used the ballast size (set manually) by subtracting it from the reported memory usage so the ballast wouldn't be counted when it calculated how much memory was being used. The ballast was actually allocated in the core of the collector (collector.go -- and it no longer is). The Splunk Otel Collector at this point just sets the environment variable SPLUNK_BALLAST_SIZE_MIB either based on the My thought was that the memory limiter processor ballast field going away is an issue, but it's perhaps a separate one. In any case, it could be handled in this PR since we're updating the collector config anyway -- we could just delete that key if we find it. Anyway, having said all that 😄 I'm happy to remove this PR's ballast extension addition code, and just remove the memory limiter ballast key (if you think that's the best way to handle its upcoming removal). And I realize that enhancing a Collector config maybe feels a bit odd at this point, but my personal opinion is that in the long term we may want to take this route to make setup easier for users. I am interested in what other folks think about this as well. |
|
Here's another option @emaderer -- I just pushed the change. This one just removes the ballast key from the memory limiter processor, if one exists, and doesn't add a ballast extension. Alternatively we could do both 😄 . |
Also remove adding ballast extension from the PR.
Can we take the approach of - if the ballast key exists in the memory limiter processor remove it and make sure we enable the ballast extension with the same value, otherwise there is no need to do anything? |
|
Ok, discussed separately with Eyal, and we're going to just remove the ballast key if it exists in the memory limiter processor, which is what this PR does in its current form. I've tested these changes against a local copy of the core collector, removed the ballast field and saw that the collector started up (verified that it didn't without these changes). |
| // converterProvider wraps a ParserProvider and accepts a list of functions that | ||
| // convert ConfigMaps. The idea is for this type to conform to the open-closed | ||
| // principle. | ||
| type converterProvider struct { |
There was a problem hiding this comment.
We may want to make use of open-telemetry/opentelemetry-collector#4078 when it's ready.
|
This looks great now I think! can we add a kill switch to disable any kind of config conversion just in case? |
|
|
||
| var ballastKeyRegexp *regexp.Regexp | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
If RemoveBallastKey is only called once is there a benefit to doing this in an init if there's only one consumer of ballastKeyRegexp?
There was a problem hiding this comment.
Yeah, there's no benefit. I'll move it to the function. (I hate init()s anyway 😛)
Thanks -- yes sure. Should it be a command line switch? |
command line switch sounds good. |
This change adds a
ballastParserProviderwhich wraps aConfigSourceParserProviderand adds amemory_ballastextension to theOtel config if one isn't already present. Doing so ensures that users
have memory ballast regardless of whether they explicitly added the
newly-developed
memory_ballastextension, which may not have existedwhen they set up their collector.