Fix unnecessary loki.process config reloads
#1809
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thampiotr
force-pushed
the
thampiotr/loki-process-reloads
branch
4 times, most recently
from
e588bfb
to
4a73654
Compare
ptodev
approved these changes
Oct 3, 2024
This is partially why I went with a different approach in #1426. I'd also noticed a few cases of mutations and I decided I could not guarantee that I can catch every single instance where this happens. I still think it's easier to copy than to do manually inspect the codebase for mutations, but if this will get the bug resolved for now, I'm ok with merging this. |
thampiotr
force-pushed
the
thampiotr/loki-process-reloads
branch
from
4a73654
to
23dc042
Compare
I agree that we should not rely on human reviews to catch this. And also many other possible errors. That's why I believe the right answer here is to have tests. If there is a validate / default values function, it should be tested. And same goes for most of other code that has any non-trivial logic. |
ptodev
pushed a commit
that referenced
this pull request
Oct 4, 2024
ptodev
pushed a commit
that referenced
this pull request
Oct 4, 2024
ptodev
added a commit
that referenced
this pull request
Oct 4, 2024
…SION file (#1828) * chore: bump yet-another-cloudwatch-exporter to v0.61.0 (#1690) * chore: bump yet-another-cloudwatch-exporter to v0.61.0 This bumps yet-another-cloudwatch-exporter to v0.61.0, which fixes [an issue with S3 metrics being reported as `NaN`](nerdswords/yet-another-cloudwatch-exporter#728). Affected metrics have a value of `0` in the scraped prometheus metrics. With the update to v0.61.0, they are reported correctly. * fixup! chore: bump yet-another-cloudwatch-exporter to v0.61.0 JobLevelMetricFields has been removed with nerdswords/yet-another-cloudwatch-exporter#1412 * fixup! chore: bump yet-another-cloudwatch-exporter to v0.61.0 * Update windows_exporter to v0.27.3 (#1785) * Update windows_exporter to v0.27.3 Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de> * Update windows_exporter to v0.27.3 Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de> --------- Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de> * build(deps): bump webpack from 5.82.1 to 5.94.0 in /internal/web/ui (#1594) Bumps [webpack](https://github.com/webpack/webpack) from 5.82.1 to 5.94.0. - [Release notes](https://github.com/webpack/webpack/releases) - [Commits](webpack/webpack@v5.82.1...v5.94.0) --- updated-dependencies: - dependency-name: webpack dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump rollup from 2.79.1 to 2.79.2 in /internal/web/ui (#1775) Bumps [rollup](https://github.com/rollup/rollup) from 2.79.1 to 2.79.2. - [Release notes](https://github.com/rollup/rollup/releases) - [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md) - [Commits](rollup/rollup@v2.79.1...v2.79.2) --- updated-dependencies: - dependency-name: rollup dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * 1687 otelcol.exporter.awss3 config fixes (#1791) * Address loki.process config reloads (#1809) * Fix logs cluster (#1716) * fix clustering for logs * Simplify code. * Add changelog. * Reorder changelog * Update VERSION --------- Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: morre <mmeyer@anaconda.com> Co-authored-by: Jan-Otto Kröpke <mail@jkroepke.de> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: cydergoth <cydergoth@gmail.com> Co-authored-by: Piotr <17101802+thampiotr@users.noreply.github.com> Co-authored-by: mattdurham <mattdurham@ppog.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
As discussed offline, this is a different implementation of #1426.
We're addressing only the mutation of configuration that is provided to
loki.process, which is a local solution to address too frequent config reloads.A more global (in the future) solution would ideally be implemented on the framework / runtime level that would prevent updating config when they didn't change. Or making configs truly immutable.
NOTE 1: mutating arguments doesn't always lead to issues, as they are passed by value (struct) to the component and to each
newXXXStagefunction. The problem starts when the arguments contain a slice, map or a pointer to another struct and it gets mutated.NOTE 2: I also caught a couple of stages not correctly applying their defaults because they were mutating a function-local copy of the configuration only.
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist