Add the option to configure memory ballast for Loki #5081

SasSwart · 2022-01-09T17:42:35Z

What this PR does / why we need it:
Adds the option to configure a memory ballast in order to enable reducing garbage collection when needed.
Which issue(s) this PR fixes:
Fixes #781

Special notes for your reviewer:
PR Based on reading this, and a comparable PR in cortex.
Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

CLAassistant · 2022-01-09T17:42:40Z

All committers have signed the CLA.

dannykopping

LGTM, thanks for this @SasSwart!
We'll definitely need some documentation about this, though. Do you mind adding that?

SasSwart · 2022-01-09T17:55:20Z

LGTM, thanks for this @SasSwart! We'll definitely need some documentation about this, though. Do you mind adding that?

On it.

Add additional information to Flag description

SasSwart · 2022-01-09T18:29:56Z

@dannykopping Done. Added more clarity to flag description as well.

dannykopping · 2022-01-09T18:44:56Z

@dannykopping Done. Added more clarity to flag description as well.

Sorry, I should've been more clear. We have our docs at https://grafana.com/docs/loki/latest/configuration/ - which is linked to the docs directory. The comments are worthwhile, for sure, so you can leave those as they are; I think we'll need an update to the yaml config overview, perhaps plus a page in Operations about how/when to use this ballast setting.

This is asking a lot so don't feel obligated; we can add that after merging the PR.

cyriltovena · 2022-01-09T18:51:29Z

I agree I don't know when I should use it either

SasSwart · 2022-01-10T00:19:07Z

This is asking a lot so don't feel obligated

No, it should definitely be on me to add docs for my contribution.
Not sure how I missed that the entire docs directory. My bad.
Adding docs

Add documentation

SasSwart · 2022-01-10T01:49:03Z

While adding documentation, I noticed that I wasn't providing a tag for unmarshalling BallastBytes from the config file.

I've moved the BallastBytes value to the root config struct instead of the ConfigWrapper and provided the tag.

I also removed the option to set this as a Command Line argument in favor of the config file.
This seems to align better with the other configuration options.

dannykopping · 2022-01-10T07:14:07Z

While adding documentation, I noticed that I wasn't providing a tag for unmarshalling BallastBytes from the config file.

I've moved the BallastBytes value to the root config struct instead of the ConfigWrapper and provided the tag.

I also removed the option to set this as a Command Line argument in favor of the config file. This seems to align better with the other configuration options.

Thanks for this.

We generally allow for the setting of important config options via both YAML and CLI; would you mind restoring this one? It can be defined in pkg/loki/loki.go under func (c *Config) RegisterFlags(f *flag.FlagSet) like we do for auth_enabled, for example.

cyriltovena

LGTM

dannykopping · 2022-01-10T09:03:03Z

Thanks @SasSwart 🙌
We'll wait for @KMiller-Grafana to approve the docs changes and then merge.

bboreham · 2022-01-10T10:19:28Z

reduce the amount of time spent resizing goroutine stack sizes.

May I ask what that relates to? Normally I would say, as the code does, it's to reduce garbage-collection.

SasSwart · 2022-01-10T10:29:54Z

Hey @bboreham

That would be a wording mistake on my side. When opening the PR, I must've referenced the wrong section of this issue. Thanks for spotting this.

I seem to have referred to the top level comment, whereas my contribution is more related to this comment.

I'll update the PR description.

sandeepsukhani

Thanks for the PR! LGTM!

KMiller-Grafana

This PR was merged in before my doc suggestions were written. Please consider using the suggestions in further revision.

Memory ballast is cool!

KMiller-Grafana · 2022-01-10T23:28:39Z

docs/sources/configuration/_index.md

@@ -93,6 +93,12 @@ Pass the `-config.expand-env` flag at the command line to enable this way of set
 # if true. If false, the OrgID will always be set to "fake".
 [auth_enabled: <boolean> | default = true]

+# The amount of virtual memory to reserve as a ballast in order to optimise


Wording suggestion:
Change

The amount of virtual memory to reserve as a ballast in order to optimise

to be

The amount of virtual memory in bytes to reserve as ballast in order to optimize

KMiller-Grafana · 2022-01-10T23:51:27Z

docs/sources/operations/scalability.md

+## Memory Ballast
+In compute constrained environments, garbage collection can become a significant factor. This can be optimised, at the expense of memory consumption, by configuring a memory ballast using the `ballast_bytes` configuration option.
+
+A larger memory ballast will mean that standard heap size volatility is less relatively significant, and therefore less likely to trigger garbage collection. This will result in lower compute overhead, but higher memory consumption, as the heap is cleaned less frequently.


We use sentence case for headers. Here's my rewrite of this section of prose:

Memory ballast

In compute-constrained environments, garbage collection can become a significant performance factor. Frequently-run garbage collection interferes with running the application by using CPU resources. The use of memory ballast can mitigate the issue. Memory ballast allocates extra, but unused virtual memory in order to inflate the quantity of live heap space. Garbage collection is triggered by the growth of heap space usage. The inflated quantity of heap space reduces the perceived growth, so garbage collection occurs less frequently.

Configure memory ballast using the ballast_bytes configuration option.

Thanks for the notes @KMiller-Grafana,

I'll prepare a follow up PR with these changes.

…/loki/pull/5081\#pullrequestreview-848460132

* Add memory ballast * Add changelog entry * Add documentation Add additional information to Flag description * Ensure that ballast_bytes can be configured from config file Add documentation * Add Operational scalability documentation related to memory ballast * Restore config.ballast-bytes cli flag * Move config.ballast-bytes cli flag declaration * Update memory ballast documentation as per https://github.com/grafana/loki/pull/5081\#pullrequestreview-848460132 Co-authored-by: Owen Diehl <ow.diehl@gmail.com>

Add memory ballast

623f96f

SasSwart requested a review from a team as a code owner January 9, 2022 17:42

pull-request-size bot added the size/XS label Jan 9, 2022

SasSwart changed the title ~~Add memory ballast~~ Add the option to configure memory ballast for Loki Jan 9, 2022

Add changelog entry

360df57

dannykopping approved these changes Jan 9, 2022

View reviewed changes

Add documentation

1ff4222

Add additional information to Flag description

pull-request-size bot added size/S and removed size/XS labels Jan 9, 2022

Ensure that ballast_bytes can be configured from config file

8102421

Add documentation

SasSwart requested a review from KMiller-Grafana as a code owner January 10, 2022 01:02

Add Operational scalability documentation related to memory ballast

6d43ca2

SasSwart added 2 commits January 10, 2022 09:55

Restore config.ballast-bytes cli flag

37631af

Move config.ballast-bytes cli flag declaration

87fb8cf

cyriltovena approved these changes Jan 10, 2022

View reviewed changes

sandeepsukhani approved these changes Jan 10, 2022

View reviewed changes

Merge branch 'main' into jjs_ballast

a61c505

owen-d approved these changes Jan 10, 2022

View reviewed changes

owen-d merged commit 79c337e into grafana:main Jan 10, 2022

KMiller-Grafana reviewed Jan 10, 2022

View reviewed changes

SasSwart referenced this pull request in SasSwart/loki Jan 12, 2022

Update memory ballast documentation as per https://github.com/grafana…

b980ccb

…/loki/pull/5081\#pullrequestreview-848460132

SasSwart mentioned this pull request Jan 12, 2022

Docs: Update memory ballast documentation #5106

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the option to configure memory ballast for Loki #5081

Add the option to configure memory ballast for Loki #5081

SasSwart commented Jan 9, 2022 •

edited

Loading

CLAassistant commented Jan 9, 2022 •

edited

Loading

dannykopping left a comment •

edited

Loading

SasSwart commented Jan 9, 2022

SasSwart commented Jan 9, 2022

dannykopping commented Jan 9, 2022

cyriltovena commented Jan 9, 2022

SasSwart commented Jan 10, 2022 •

edited

Loading

SasSwart commented Jan 10, 2022

dannykopping commented Jan 10, 2022

cyriltovena left a comment

dannykopping commented Jan 10, 2022

bboreham commented Jan 10, 2022

SasSwart commented Jan 10, 2022

sandeepsukhani left a comment

KMiller-Grafana left a comment

KMiller-Grafana Jan 10, 2022 •

edited

Loading

KMiller-Grafana Jan 10, 2022

SasSwart Jan 11, 2022

Add the option to configure memory ballast for Loki #5081

Add the option to configure memory ballast for Loki #5081

Conversation

SasSwart commented Jan 9, 2022 • edited Loading

CLAassistant commented Jan 9, 2022 • edited Loading

dannykopping left a comment • edited Loading

Choose a reason for hiding this comment

SasSwart commented Jan 9, 2022

SasSwart commented Jan 9, 2022

dannykopping commented Jan 9, 2022

cyriltovena commented Jan 9, 2022

SasSwart commented Jan 10, 2022 • edited Loading

SasSwart commented Jan 10, 2022

dannykopping commented Jan 10, 2022

cyriltovena left a comment

Choose a reason for hiding this comment

dannykopping commented Jan 10, 2022

bboreham commented Jan 10, 2022

SasSwart commented Jan 10, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

KMiller-Grafana left a comment

Choose a reason for hiding this comment

KMiller-Grafana Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

KMiller-Grafana Jan 10, 2022

Choose a reason for hiding this comment

Memory ballast

SasSwart Jan 11, 2022

Choose a reason for hiding this comment

SasSwart commented Jan 9, 2022 •

edited

Loading

CLAassistant commented Jan 9, 2022 •

edited

Loading

dannykopping left a comment •

edited

Loading

SasSwart commented Jan 10, 2022 •

edited

Loading

KMiller-Grafana Jan 10, 2022 •

edited

Loading