[helm]: implement system integration as chart built-in #5855
Conversation
pkoutsovasilis
added
enhancement
ycombinator
requested review from
swiatekm and
blakerouse
and removed request for
kaanyalti and
pchila
| ### 4 - System integration | ||
| | Key | Type | Default | Description | | ||
| |-----|------|---------|-------------| | ||
| | system.enabled | bool | `false` | enable System integration. | |
There was a problem hiding this comment.
Why is the default false? I feel like most will want this on, it brings immediate value to the Observability section in Kibana.
There was a problem hiding this comment.
More than happy to have it by default as true, it also makes sense to me. @nimarezainia what do you think on that? 🙂
deploy/helm/elastic-agent/templates/integrations/_system/_system_logs.tpl
Outdated
Show resolved
Hide resolved
deploy/helm/elastic-agent/templates/integrations/_system/_system_logs.tpl
Outdated
Show resolved
Hide resolved
pierrehilbert
added
the
Team:Elastic-Agent-Control-Plane
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
pkoutsovasilis
force-pushed
the
feat/helm_chart_system_integration
branch
from
64e4512
to
70205a8
Compare
thanks for the review @swiatekm and the example proposal, I took inspiration from one of @blakerouse 's comments and added one but also did some other changes, would you mind having another look? 🙂 |
| @@ -0,0 +1,9 @@ | |||
| system: | |||
There was a problem hiding this comment.
We should have rendered examples for this, right? I thought the mage target would error in this case.
There was a problem hiding this comment.
hmm, I thought I added it but this never happened, but the CI should fail yes
There was a problem hiding this comment.
oh I suspect that this is because the file was never versioned and the check-ci apparently fails on changes of versioned files?!
There was a problem hiding this comment.
Shouldn't it fail anyway if it can't find the file? I didn't know the examples mage target was aware of versioning.
Anyway, this isn't a blocker for this PR, but we should make sure the check actually catches issues.
There was a problem hiding this comment.
the current CI checks flow is the following:
- render helm examples
- check for git diffs; the assumption here was that it failed also on unversioned files but evidently that's not the case
100% agreed this needs to be extended
| dataset: system.diskio | ||
| type: metrics | ||
| period: 10s | ||
| diskio.include_devices: null |
There was a problem hiding this comment.
Are these nulls intentional? I would expect this to either be an empty list or for the key to not exist at all.
There was a problem hiding this comment.
this crossed my mind as well, but then I wondered why this config was rendered like that from Kibana; thus, I figured not to mess with it 😄 but sure this can be further optimised in the future
|
What does this PR do?
This PR introduces support for the system integration
Why is it important?
system integration is usually used in the context of k8s, thus having it as a built-in integration improves the user configuration experience
Checklist
./changelog/fragmentsusing the changelog toolDisruptive User Impact
How to test this PR locally