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

chore: add function to group entries by action in changeset output #844

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

94DanielBrown
Copy link
Contributor

This can then be used to improve cardinality of logs in kustomize-controller.
I have tested and will make a PR to kustomize-controller which does this:

log.Info("server-side apply for cluster definitions completed", "output", changeSet.ToMap())
+                       log.Info("server-side apply for cluster definitions completed", "output", changeSet.ToGroupedMap()

It helps with fluxcd/flux2#4036

@stefanprodan
Copy link
Member

We can't just change the log format of a GA controller, we'll break the filters people have for these logs just to comply with a small subset that are using elastic. If we decide to get this in, it needs to be under a feature flag in kustomize-controller.

@94DanielBrown
Copy link
Contributor Author

Yeah that makes sense

But is there any harm getting the function added into here for now and then I can look at adding the feature flag in the controller and using it there after, if you agree that's an okay thing to do?

@stefanprodan
Copy link
Member

stefanprodan commented Dec 13, 2024

We need to analyse all the logging across all controllers and check if the structure logs contain keys with namespace/name/kind/etc. Changing this one log line may not solve the issue with ES.

@94DanielBrown
Copy link
Contributor Author

Can it not just be added as a feature flag for just the kustomize controller as this is where the issue are?

Also if it's added here as a function it doesn't have to be used but it makes it easier to use if someone want to run a fork of the controller to sort out the logging in ES/OS ?

@stefanprodan
Copy link
Member

Can it not just be added as a feature flag for just the kustomize controller as this is where the issue are?

Yes, what I'm saying is that there may be other places where the log keys are high cardinality, and the feature flag should fix this everywhere. I'll run some queries on my clusters and see if such logs are in other places besides here.

@stefanprodan
Copy link
Member

@94DanielBrown if you have time this week to help with this we can include this in Flux 2.5 release. Based on my log analysis, kustomize-controller apply and delete logs could be put behind a feature flag to switch the format. Please rebase this PR.

@stefanprodan
Copy link
Member

@94DanielBrown can you please rebase and force push, this PR should have a single commit.

@stefanprodan stefanprodan added the area/server-side-apply SSA related issues and pull requests label Feb 11, 2025
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stefanprodan stefanprodan merged commit 00d1ceb into fluxcd:main Feb 11, 2025
12 checks passed
@stefanprodan
Copy link
Member

I've tagged this as ssa/v0.45.1 so you can bump the ssa package in kustomize-controller and add a feature flag to switch the log format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/server-side-apply SSA related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants