Disable mem-ballast-size-mib from command line option

[mxiamxia]

Description: <Describe what has changed.

• Disable mem-ballast-size-mib command line option for setting memory ballast

Link to tracking Issue:
#2516

[bogdandrutu]

This is a breaking change:

1. Needs to be added to CHANGELOG
2. How are we going to signal the users? Is it possible to keep the flag for few versions, then log an warning and hardcode to create the extension for the user?

[mxiamxia]

This is a breaking change:

2. How are we going to signal the users? Is it possible to keep the flag for few versions, then log an warning and hardcode to create the extension for the user?

Can we keep mem-ballast-size-mib option in command line but do nothing on it for a few releases. I will add the warning msg to ask users to switch to ballast extension as well.

ballast is mainly for improving CPU usage . I don't think it make a big impact to users, if they noticed worse CPU usage on Collector process after the upgrade, they should come back to check CHANGELOG.

[mxiamxia]

cc/ @bogdandrutu PTAL!

[mxiamxia]

Load test failed due to the dependency on #3625

/home/circleci/project/testbed/tests/results/TestBallastMemory.

[bogdandrutu]

Is not deprecated, is removed correct? Since the behavior is no longer there. Consider to explain what to do to fix this.

[mxiamxia]

Corrected the wordings. Thanks!

[alolita]

@Aneurysm9 @jrcamp can you please review this PR too? Thx.

[jrcamp]

Isn't this the same as #3493? See #3493 (review)

[mxiamxia]

Isn't this the same as #3493? See #3493 (review)

Hi @jrcamp, thanks for pointing it out, it is pretty much the same. I'm fine to merge either PR. I think the blocker is how we'll handle the breaking change and move fwd. I'll raise it in SIG meeting.

[tigrannajaryan]

Duplicate of #3493
Since this a breaking change for our distro with many existing installations let's give some graceful deprecation period before we actually remove the feature. I suggest we return to this in 3 months.
For this PR: instead of removing the feature make it deprecated for now (give a warning, document that it is deprecated, announce in SIG and in Slack, etc).

[bogdandrutu]

@tigrannajaryan as we discussed here, this is not a breaking change. The downgrade in performance (if ballast is not correctly set) cannot be considered a breaking change.

[tigrannajaryan]

@tigrannajaryan as we discussed here, this is not a breaking change. The downgrade in performance (if ballast is not correctly set) cannot be considered a breaking change.

I think it will cause the Collector to fail at statup when the command line option is removed but someone tries to use it (as it is used in Splunk distro). If that is the case then I think it can be considered a breaking change, no?

[bogdandrutu]

@tigrannajaryan I am not sure how flags are implemented, but indeed we should not FAIL, but it is fine to just print a warning and not have ballast setup if using the flag. That is what I understood from that comment.

@mxiamxia can you test that? We may need to leave the flag and if set we log an warning and done :)

[alolita]

@mxiamxia can you test the flag conditionality asap. Let's get this PR done. :-)

[mxiamxia]

@tigrannajaryan I am not sure how flags are implemented, but indeed we should not FAIL, but it is fine to just print a warning and not have ballast setup if using the flag. That is what I understood from that comment.

@mxiamxia can you test that? We may need to leave the flag and if set we log an warning and done :)

Hi @bogdandrutu, yes, we keep the flag in the command line but does nothing so we don't have the breaking change. The change has been verified. :)

cc/ @alolita

[tigrannajaryan]

I don't understand what's the rush with this change? It is still a performance degradation for existing users. What prevents us from keeping the existing behavior for a while?

[bogdandrutu]

@tigrannajaryan if you block this you will maintain it (because will be a backwards incompatible change) for the next 10 years, please sign with blood and will not push on this :)

Jokes aside, because it is very bad how we designed the collector to have half things in YAML and the other half in flags. We have 0 consistency of when things are configure where.

[tigrannajaryan]

@tigrannajaryan if you block this you will maintain it (because will be a backwards incompatible change) for the next 10 years, please sign with blood and will not push on this :)

Jokes aside, because it is very bad how we designed the collector to have half things in YAML and the other half in flags. We have 0 consistency of when things are configure where.

I agree, I don't mind removing it. I am just saying let's give some grace period for users before we delete it. We are only declaring traces stable so we have no obligation to keep this for 10 years after Trace GA.

I may be missing something.

[mxiamxia]

@tigrannajaryan if you block this you will maintain it (because will be a backwards incompatible change) for the next 10 years, please sign with blood and will not push on this :)
Jokes aside, because it is very bad how we designed the collector to have half things in YAML and the other half in flags. We have 0 consistency of when things are configure where.

I agree, I don't mind removing it. I am just saying let's give some grace period for users before we delete it. We are only declaring traces stable so we have no obligation to keep this for 10 years after Trace GA.

I may be missing something.

Hi @tigrannajaryan , since the current change won't break the existing collector instances, I think we should merge this changes before the trace GA release. In another way, ballast is mainly for improving CPU usage . I don't think it will make a big impact to the current users, if they noticed a worse CPU usage on Collector after the upgrade, they should come back to check CHANGELOG.

[tigrannajaryan]

@mxiamxia we discussed with Bogdan and he is going to update our distro to use the new ballast approach (extension). Once it is done we should be able to move forward.

[mxiamxia]

@mxiamxia we discussed with Bogdan and he is going to update our distro to use the new ballast approach (extension). Once it is done we should be able to move forward.

Sounds good! Thank you for the update.