add support for queue settings under outputs by leehinman · Pull Request #36788 · elastic/beats

leehinman · 2023-10-06T22:30:14Z

Proposed commit message

add support for queue settings under output. Validation ensure only top level or output level is specified.

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.

Author's Checklist

[ ]

How to test this PR locally

Stand alone Filebeat

Start Filebeat with following config file:

output.elasticsearch:
  queue.mem:
    events: 1024
    flush.min_events: 2
    flush.timeout: 15s

Should show queue.max_events in the metrics to be 1024.

Agent

Build metricbeat and filebeat from this branch.
Build or download an 8.11 snapshot agent.
Copy the built metricbeat and filebeat into the data/elastic-agent-$hash/components directory of the agent.
Install the agent. Enrolled it with Fleet (standalone agent should work too)
Configure the queue as shown above
Check logs that queue.max_events in the metrics is 1024

Related issues

Closes Allow queue configuration to be specified under the output type #35615

mergify · 2023-10-06T22:30:50Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @leehinman? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2023-10-06T23:20:07Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-10-19T21:54:11.727+0000
Duration: 70 min 0 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28482
Skipped	2013
Total	30495

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

leehinman · 2023-10-10T15:02:25Z

Sorry, I had trouble re-opening #36693, so this a new PR number but is really a continuation.

ycombinator · 2023-10-11T00:19:32Z

libbeat/cmd/instance/beat_test.go

+	tests := map[string]struct {
+		input                 []byte
+		memEvents             int
+		expectValidationError bool


Is this field being used?

nope, thanks.

ycombinator · 2023-10-11T00:21:06Z

libbeat/cmd/instance/beat_test.go

+      mem:
+        events: 8096
+`),
+			memEvents: 8096},


Nit: Could totally be just me, but I found these test cases a bit hard to read. I think it might've helped to have the opening and closing braces of each test case struct on their own lines so the struct fields all lined up nicely.

See if the new fmt is better. I'm open to any suggestions, I just want to be sure the yaml looks like yaml.

ycombinator · 2023-10-11T00:36:37Z

libbeat/outputs/util.go


 // Success create a valid output Group response for a set of client instances.
-func Success(batchSize, retry int, clients ...Client) (Group, error) {
+func Success(cfg config.Namespace, batchSize, retry int, clients ...Client) (Group, error) {


Let's document here that cfg is expected to contain queue configuration, since its datatype here config.Namespace doesn't quite make that obvious.

Same for SuccessNet below.

ycombinator · 2023-10-11T00:45:53Z

libbeat/publisher/queue/memqueue/broker.go

 	logger *logp.Logger,
 	ackCallback func(eventCount int),
 	settings Settings,
+	inputQueueSize int,


It's a bit odd not to have inputQueueSize be part of settings as before. I agree with removing the special override in the publisher pipeline code but I think that can be done while leaving inputQueueSize as part of settings so this constructor here has still has the same signature as before, with all queue settings being in settings. For that you'll need to set settings.InputQueueSize wherever you call this constructor from (unless, of course, you want to leave that field at it's zero-value, e.g. in test code).

The only thing I don't like about that is it is hard to tell if you are supposed to set settings.InputQueueSize before you pass it into FactoryForSettings or allow FactoryForSettings to set it. Also what happens if you pass in a Settings with that field set? Do you override or keep?

I don't like changing the signature, but I'm not fond of the ambiguous nature of who sets InputQueueSize if we leave it in settings. I can be convinced to go either way, do you have a strong opinion?

ycombinator · 2023-10-11T00:48:37Z

This PR seems to consist of three types of unrelated changes: some refactoring/cleaning, queue setting changes, idle connection timeout setting changes. That's making it a bit hard to review the impact of each change in isolation.

Would it be a lot of work to break this up into three smaller PRs? In general, I think that's a better approach, not just to make reviews easier but also to allow rolling back changes in isolation, if necessary.

leehinman · 2023-10-11T14:53:20Z

This PR seems to consist of three types of unrelated changes: some refactoring/cleaning, queue setting changes, idle connection timeout setting changes. That's making it a bit hard to review the impact of each change in isolation.

Would it be a lot of work to break this up into three smaller PRs? In general, I think that's a better approach, not just to make reviews easier but also to allow rolling back changes in isolation, if necessary.

Good idea, I'll split out the idle connection timeout. Unfortunately all the other changes are necessary for the queue under outputs change. I'll add some more comments as to why.

leehinman · 2023-10-13T19:04:04Z

libbeat/cmd/instance/beat.go

 	return validIPs
 }
+
+func promoteOutputQueueSettings(bc *beatConfig) error {


promoteOutputQueueSettings handles the use case where the beat is started with a configuration file that has outputs defined (normal stand alone beat).

leehinman · 2023-10-13T19:46:36Z

#36843 now contains the idle_connection_timeout changes.

ycombinator · 2023-10-13T20:22:16Z

libbeat/outputs/elasticsearch/elasticsearch.go

 	log := logp.NewLogger(logSelector)
 	if !cfg.HasField("bulk_max_size") {
-		cfg.SetInt("bulk_max_size", -1, defaultBulkSize)
+		_ = cfg.SetInt("bulk_max_size", -1, defaultBulkSize)


You might have a merge conflict here once #36843 is merged.

- add support for `idle_connection_timeout` for ES output - add support for queue settings under output Closes elastic#35615

ycombinator

LGTM.

bschaeffer · 2024-01-25T17:33:29Z

This is a confusing change for folks building custom output plugins. Where do I put the configuration for queues now?

# global
queue.mem:
  events: 1024

# output local
output.custom:
  queue.mem:
    events: 1024

If I can leave them as a global config... how do I access the config using the output factory function:

beats/libbeat/outputs/output_reg.go

Lines 31 to 35 in 433a1a1

    
           type Factory func( 
        
           	im IndexManager, 
        
           	beat beat.Info, 
        
           	stats Observer, 
        
           	cfg *config.C) (Group, error)

bschaeffer · 2024-01-25T18:32:45Z

Looks like only one is allowed:

error unpacking config data: top level queue and output level queue settings defined, only one is allowed accessing config

* add support for queue settings under outputs

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 6, 2023

mergify bot assigned leehinman Oct 6, 2023

leehinman force-pushed the 35615_queue_under_output branch 2 times, most recently from 441c87c to 52b0111 Compare October 6, 2023 23:08

leehinman force-pushed the 35615_queue_under_output branch 4 times, most recently from 4b506ee to ca41e03 Compare October 9, 2023 19:43

leehinman marked this pull request as ready for review October 9, 2023 21:43

leehinman requested review from a team as code owners October 9, 2023 21:43

leehinman requested review from fearful-symmetry and ycombinator October 9, 2023 21:43

pierrehilbert added the Team:Elastic-Agent Label for the Agent team label Oct 10, 2023

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 10, 2023

ycombinator reviewed Oct 11, 2023

View reviewed changes

leehinman commented Oct 13, 2023

View reviewed changes

ycombinator reviewed Oct 13, 2023

View reviewed changes

add support for queue settings under outputs

95ded73

- add support for `idle_connection_timeout` for ES output - add support for queue settings under output Closes elastic#35615

leehinman added 10 commits October 19, 2023 11:00

update changelog with PR number

1c1c160

fix lint check

d6e8568

add config validation

008f9a8

remove doc about top level precedence

270499b

add check for underAgent and DiskQueue

95679d6

catch elastic-agent and disk queue at validation

422fe00

support for reloading

b1a8666

remove idle_connection_timeout

a4a5843

respond to comments

d075980

Add doc strings to help explain why changes are the way they are

4dc26ba

leehinman force-pushed the 35615_queue_under_output branch from 505fa7c to 4dc26ba Compare October 19, 2023 18:39

fix NOTICE.txt

b005670

ycombinator approved these changes Oct 19, 2023

View reviewed changes

Merge branch 'main' into 35615_queue_under_output

75d9daf

leehinman enabled auto-merge (squash) October 19, 2023 21:54

leehinman merged commit 0bd2d73 into elastic:main Oct 19, 2023

leehinman deleted the 35615_queue_under_output branch October 30, 2023 14:08

Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024

add support for queue settings under outputs (elastic#36788)

6ac71ce

* add support for queue settings under outputs

This was referenced Jan 15, 2025

[Documentation] Improve docs on adding queue settings under output #42310

Closed

[filebeat] Add support for bulk_max_size and preset #42312

Merged

Conversation

leehinman commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed commit message

Checklist

Author's Checklist

How to test this PR locally

Stand alone Filebeat

Agent

Related issues

Uh oh!

mergify bot commented Oct 6, 2023

Uh oh!

elasticmachine commented Oct 6, 2023 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

leehinman commented Oct 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Oct 11, 2023

Uh oh!

leehinman commented Oct 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leehinman commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

bschaeffer commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bschaeffer commented Jan 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leehinman commented Oct 6, 2023 •

edited

Loading

elasticmachine commented Oct 6, 2023 •

edited by jenkins-beats-ci bot

Loading

bschaeffer commented Jan 25, 2024 •

edited

Loading