feat(helm): permit configuration of exporter.performance settings#46
feat(helm): permit configuration of exporter.performance settings#46x10an14-nav wants to merge 2 commits intosoftwaremill:mainfrom
exporter.performance settings#46Conversation
edd67ee to
ed875af
Compare
|
Finished polishing the code (we branched on top of this branch to deploy our own helm-chart until this gets merged). Should be good to go when you guys are happy w/it. PS: The |
|
PPS: Using static libs instead of dynamic libs the binary & docker image size can get heavily reduced, even further: Nix already reduces image by only importing sw dependencies & building |
ed875af to
76fd872
Compare
Maybe missed as part of softwaremill#41 ?
eg. `helm template . --set-json '{"log_level": "trace,rdkafka=info,axum=info"}'`
76fd872 to
63b49b4
Compare
|
Rebased on latest |
softberries
left a comment
There was a problem hiding this comment.
The Helm chart changes are correct and needed — [exporter.performance] was indeed missing from the configmap template, making those settings impossible to tune via Helm values. The log_level → RUST_LOG env var is a reasonable convenience. Defaults look good and match the application defaults.
A few things worth addressing:
1. Split Nix files into a separate PR
The Nix flake files (.envrc, flake.nix, flake.lock, .gitignore) are a developer environment setup — completely unrelated to the Helm chart fix. The Helm change is ~20 lines across 3 files; the Nix stuff is 230+ lines (mostly lock file) that obscures the actual change. These should be a separate PR for cleaner review and history.
2. Missing nil-guard on configmap.yaml
[exporter.performance]
{{- with index .Values.config "exporter.performance" }}If someone removes the exporter.performance section from their values override, this will render [exporter.performance] with empty/nil fields and produce an invalid config. Should wrap the entire block (including the section header) in a conditional:
{{- with index .Values.config "exporter.performance" }}
[exporter.performance]
kafka_timeout = {{ .kafka_timeout | quote }}
...
{{- end }}3. log_level placement
log_level is a top-level value rather than nested under config.exporter where it logically belongs. It also won't interact cleanly with the existing envFrom patterns in the deployment template — if someone sets RUST_LOG via envFromConfigMaps or envFromSecrets, there'd be a conflict with undefined precedence. Consider either nesting it or documenting the precedence.
|
Hi @softberries! Thanks for replying to my PR! If I am alone spending time writing human-to-human communication manually, there's not much incentive for me to continue to do so moving forwards. By all means, if that's how you'd like to set the playbook for this repo, that's your right and prerogrative - I just fear it'll end up being an "LLMs discussing via-humans-as-proxy" situation, that (without knowing you/having interacted with you before) I imagine you too would like to avoid. I say the above since I know with near certainty that I could get my LLM to write a detailed reasonable rebuttal to most if not every point raised/sentence you wrote (not that I want to, I'd rather keep the intra-human communication LLM free). |
|
If I am mistaken in my assumption that your reply was LLM based, I'd like to own that and apologize. |
|
Hello @x10an14-nav , yes the previous reply was AI generated as we are using multiple tools to communicate on GitHub and review some of the PRs. They are however reviewed before posting so it would be the same if eg. I would write it myself. We don't want to block you from contributions and we believe that the part not related to nix is needed and we want to introduce it anyway. If you still want to proceed with this PR that would be great, if on the other hand you don't want to do it we will add those changes ourselves as we need it and it should be added earlier. One way or the other its great to have you using klag-exporter and having you here. Regards |
Maybe missed as part of #41 ?