Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unnecessary loki.process config reloads #1426

Closed
wants to merge 3 commits into from

Conversation

ptodev
Copy link
Contributor

@ptodev ptodev commented Aug 6, 2024

PR Description

When Alloy's config is reloaded, components whose config didn't change should not be reloaded. This hasn't been true for loki.process if certain stages are present. That's because for some stages the config struct is mutated after the pipeline is started. Notably, this is the case for stage.labels. The solution I went with in this PR is to deep copy the config struct so that we can compare the old and new configs more reliably.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@thampiotr
Copy link
Contributor

I'm a bit hesitant about this change as it adds a lot of code that could be considered a boilerplate and an extra maintenance burden. IIRC this is also code that we copied from loki/promtail so it creates a further diversion. Could we have one way to solve it for all configs? for example, serialise these structures into string alloy config and compare these to detect changes? or use some other way?

@ptodev
Copy link
Contributor Author

ptodev commented Aug 13, 2024

@thampiotr thank you very much for the review!

IIRC this is also code that we copied from loki/promtail so it creates a further diversion. Could we have one way to solve it for all configs?

That's true. To avoid diversion, I could make the same change in the Promtail code in the loki repo. I'm not sure if it's necessary there though. Promtail does have a config reload feature, but I'm not sure how (and if) the old config is compared with the new one.

serialise these structures into string alloy config and compare these to detect changes? or use some other way?

Unfortunately, serialisation to a string doesn't work because things like secrets have custom behaviour and they'll be redacted in the final string. So if the config changes, and only a secret has changed, a config reload wouldn't happen (which would be wrong). I think the Agent has this bug in static mode. If you have other ideas I'd be happy to hear them, but this is the best I came up with. I realise it's a lot of code, I also am not a huge fan of this :/ But it's reliable.

@thampiotr
Copy link
Contributor

Unfortunately, serialisation to a string doesn't work because things like secrets have custom behaviour and they'll be redacted in the final string.

Good point. But maybe it's worth adding functionality to serialise with secrets or at least hash of the secrets so that we can avoid the hand-written code to do all these cloning?

@ptodev
Copy link
Contributor Author

ptodev commented Aug 14, 2024

Good point. But maybe it's worth adding functionality to serialise with secrets or at least hash of the secrets so that we can avoid the hand-written code to do all these cloning?

Is this more reliable than simply having Copy() functions? There could be other types which have custom marshal behaviour too. I'm not confident that we can track all of these and to keep the list up to date.

Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants