opentelemetry-helm-charts icon indicating copy to clipboard operation
opentelemetry-helm-charts copied to clipboard

set proper resources for collector

Open markandersontrocme opened this issue 1 year ago • 7 comments

Closes #272

markandersontrocme avatar Jul 11 '22 19:07 markandersontrocme

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: MarkAndersonTrocme / name: Mark Anderson-Trocme (9177389cdf01605105fc1b4f0000faee8c3e8d65)

@MarkAndersonTrocme pls bump the chart version. Also I would tend to give a bit more memory for limits.

Allex1 avatar Jul 11 '22 20:07 Allex1

@Allex1 Let me know if those resources make more sense now? If not let me know what you recommend

markandersontrocme avatar Jul 12 '22 12:07 markandersontrocme

@dmitryax @MarkAndersonTrocme @Allex1 I support this change but I think its a "breaking change". Some customers may be relying on the higher default values and if they upgraded to this version their requests/limits would get halved. Any ideas on how we can handle that gracefully?

TylerHelmuth avatar Jul 13 '22 15:07 TylerHelmuth

We increase the minor version and maybe update the main Readme with a warning? While the chart suggests sensible defaults users are responsible for setting their own resource allocation values. Also users should expect some imminent breaking changes as the chart is not yet stable. Once it's stable we could increase the major version when making breaking changes and document the upgrade path on our main Readme. wdyt?

Allex1 avatar Jul 13 '22 15:07 Allex1

Ya without a major version I can't think of a good way to handle this without introducing a new field, which I don't really want to do. We do have an UPGRADING.md doc that feels like a good place to callout this change. @MarkAndersonTrocme can you add a new entry in UPGRADING.md that calls out this change and shows readers how to set the values back to the old values?

TylerHelmuth avatar Jul 18 '22 14:07 TylerHelmuth

@MarkAndersonTrocme please handle merge conflicts.

TylerHelmuth avatar Jul 22 '22 15:07 TylerHelmuth

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Aug 09 '22 02:08 github-actions[bot]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions[bot] avatar Aug 23 '22 03:08 github-actions[bot]

@TylerHelmuth @dmitryax I've reopened this as I think it's useful to set lower sensible defaults. PTAL

Allex1 avatar Aug 26 '22 08:08 Allex1