-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Inject inferred queue settings into agent config generation #27429
Conversation
Pinging @elastic/agent (Team:Agent) |
This pull request doesn't have a |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
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 think one part needs to be fixed. Otherwise looks good.
name: "hello", | ||
value: &StrVal{value: "fae was here"}, | ||
} | ||
if err := Insert(ast, faeNode, "output"); err != nil { |
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.
Should this be here at all? Was this a debugging thing?
I think if no output is defined it should just return nil and do nothing.
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.
Haha yes, leftover from when I was still figuring out how to traverse ASTs, removed
name: "min_events", | ||
value: &IntVal{value: min_events}, | ||
}, | ||
// flush.timeout defaults to 1s, which is what we want for now, |
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.
should we always set the default by Elastic Agent? that way if libbeat changes that doesn't affect what Elastic Agent expects?
no hard requirement to change this behavior, its just a thought
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 debated that... this has been a stable parameter but yea, let's make it explicit. Done.
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.
Thanks for the fixes. Looks good.
(cherry picked from commit 9e6c6e1)
(cherry picked from commit 9e6c6e1)
What does this PR do?
Add a new internal rule,
inject_queue
, to agent's config transpilation process, which derives implicit memory queue parameters from the user-specified output settings and injects them into the generated configuration, and call the rule from the appropriate spec files. See #26638 for details on the reasoning behind this feature.I haven't changed the documentation or default config files, since this change doesn't add any new user-customizable settings, it just adapts the backend settings better to the context. There should be a followup PR that clarifies that queue behavior is affected by output settings
bulk_max_size
andworker
, although we're intentionally not committing to an exact conversion formula since we may want to refine it in the future.Resolves #26638.
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.