-
Notifications
You must be signed in to change notification settings - Fork 143
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
[helm]: implement system integration as chart built-in #5855
base: main
Are you sure you want to change the base?
[helm]: implement system integration as chart built-in #5855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any issue, just really a comment on it being default and extra options. Not really blockers, they can be adjusted later if we see that it should be default or those settings need options.
### 4 - System integration | ||
| Key | Type | Default | Description | | ||
|-----|------|---------|-------------| | ||
| system.enabled | bool | `false` | enable System integration. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I don't have an informed opinion on whether this integration should be enabled by default. But if it's not, can we add an example where we enable it?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I thought I added it but this never happened, but the CI should fail yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the added configurability. Had a small question about the configuration template, but it shouldn't block merging this PR. Nice work. 💪
dataset: system.diskio | ||
type: metrics | ||
period: 10s | ||
diskio.include_devices: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Quality Gate passedIssues Measures |
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/fragments
using the changelog toolDisruptive User Impact
How to test this PR locally