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

Inject inferred queue settings into agent config generation #27429

Merged
merged 7 commits into from
Aug 17, 2021

Conversation

faec
Copy link
Contributor

@faec faec commented Aug 17, 2021

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 and worker, although we're intentionally not committing to an exact conversion formula since we may want to refine it in the future.

Resolves #26638.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@faec faec added enhancement Team:Elastic-Agent Label for the Agent team backport-v8.0.0 Automated backport with mergify backport-v7.15.0 Automated backport with mergify backport-v7.16.0 Automated backport with mergify labels Aug 17, 2021
@faec faec requested a review from blakerouse August 17, 2021 17:23
@faec faec self-assigned this Aug 17, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Aug 17, 2021
@botelastic
Copy link

botelastic bot commented Aug 17, 2021

This pull request doesn't have a Team:<team> label.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 17, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-17T18:53:07.125+0000

  • Duration: 96 min 10 sec

  • Commit: 2491f0e

Test stats 🧪

Test Results
Failed 0
Passed 7028
Skipped 16
Total 7044

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 7028
Skipped 16
Total 7044

Copy link
Contributor

@blakerouse blakerouse left a 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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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,
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@blakerouse blakerouse left a 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.

@faec faec merged commit 9e6c6e1 into elastic:master Aug 17, 2021
@faec faec deleted the agent-queue branch August 17, 2021 20:29
mergify bot pushed a commit that referenced this pull request Aug 17, 2021
mergify bot pushed a commit that referenced this pull request Aug 17, 2021
faec added a commit that referenced this pull request Aug 17, 2021
…27436)

(cherry picked from commit 9e6c6e1)

Co-authored-by: Fae Charlton <fae.charlton@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify backport-v7.16.0 Automated backport with mergify backport-v8.0.0 Automated backport with mergify enhancement Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support customization of throughput relevant settings in APM integration
3 participants