x-pack/filebeat/input/http_endpoint: make input GA #39410

efd6 · 2024-05-06T04:15:24Z

Proposed commit message

See title.

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.

Disruptive User Impact

Author's Checklist

None of the linter suggestions have any merit.

The timeout ACK behaviour is tested manually using elastic-package using the the version here injected into the stack.

$ curl -X POST http://localhost:8080/ -d '{"data":1}' -H 'Content-Type: application/json'
{"message":"ok!"}
$ curl -X POST http://localhost:8080/?wait_for_completion_timeout=1m -d '{"data":1}' -H 'Content-Type: application/json'
{"message":"ok!"}
$ curl -X POST http://localhost:8080/?wait_for_completion_timeout=1s -d '{"data":1}' -H 'Content-Type: application/json'
{"message":"could not publish event within timeout"}

The first instance without a timeout returned immediately, the second took ~5s and the last ~2s. These are consistent with expected behaviour.

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify · 2024-05-06T04:16:07Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @efd6? 🙏.
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 · 2024-05-06T04:53:26Z

💚 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

Duration: 137 min 30 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 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!)

elasticmachine · 2024-05-08T00:38:36Z

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

andrewkroh · 2024-05-08T17:22:39Z

x-pack/filebeat/input/http_endpoint/ack.go

+}
+
+// Ready signals that the batch has been fully consumed. Only
+// after the batch is marked as "ready" can the lumberjack batch


~~lumberjack~~ batch

andrewkroh · 2024-05-08T18:13:58Z

x-pack/filebeat/input/http_endpoint/handler.go

@@ -124,7 +128,8 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			}
 		}

-		if err = h.publishEvent(obj, headers); err != nil {
+		acker.Add()


With end-to-end acknowledgements the goal is to be able to provide a delivery guarantee that the event(s) was written to the destination output. So I think this needs to relay the feedback to the caller that the event was written to the output.

I envision this meaning that a request blocks until the input receives all of the ACKs that it expects. This leads to a few follow-up behavioral questions:

Introducing blocking behavior could be a viewed as a breaking change. Perhaps the waiting behavior should be opt-in and have a timeout like via ?wait_for_completion_timeout=30.

Should there be any "circuit breakers" and how should the API respond when it overloaded (e.g. receiving more requests or more events than it can process in a timely manner)? We want to avoid a DoS situation if we have some high volume clients and an output that is slow or down. (This one is relevant even without E2E ack'ing.)

I considered that, but it means that we are keeping connections open while some (unrelated) internal process may block (I think that this is partially your point in 2.).

I think that this is a dangerous thing to do in the general case, but I also think that it would be reasonable to do as an opt-in that the user can choose in the configuration. This has the happy consequence that this would non-breaking. Note that I had not read 1. before I wrote this, so these are independent discoveries (honest). Looking at 1., I think that having a timeout would be sensible. So, suggest configuring based on the presence of a timeout; if present, then the request will block until the ACK or the timeout (and add a timeout failing metric), if absent do not block. This intentionally does not add a block without timeout (wait forever) since it does not make any sense in the real world.

WDYT?

That sounds good to me.

andrewkroh · 2024-05-08T19:58:49Z

x-pack/filebeat/input/http_endpoint/input.go

@@ -391,17 +402,21 @@ func newInputMetrics(id string) *inputMetrics {
 		apiErrors:           monitoring.NewUint(reg, "api_errors_total"),
 		batchesReceived:     monitoring.NewUint(reg, "batches_received_total"),
 		batchesPublished:    monitoring.NewUint(reg, "batches_published_total"),
+		batchesACKedTotal:   monitoring.NewUint(reg, "batches_acked_total"),


Reminder to add the new metrics to the table in the docs.

I just noticed that the lumberjack input does not have any docs.

See my comment under Author's Notes at #32175 (comment). Whether or not to formally advertise it is something I still debate. Feedback welcomed. It is only used and exposed as part of the barracuda integration at the moment.

Thanks, that makes perfect sense.

mergify · 2024-05-08T20:32:43Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 38979-http_endpoint upstream/38979-http_endpoint
git merge upstream/main
git push upstream 38979-http_endpoint

mergify · 2024-05-09T00:11:53Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 38979-http_endpoint upstream/38979-http_endpoint
git merge upstream/main
git push upstream 38979-http_endpoint

elasticmachine · 2024-05-10T06:30:08Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

belimawr · 2024-05-14T20:47:55Z

x-pack/filebeat/input/http_endpoint/handler.go

+			var k string
+			for k = range q {
+				break
+			}


That confused me a little bit, the for loop is there to easily extract the key from the map q, is that correct?

Could you add a little comment to make it clear why using a for loop on a map of length 1?

I would have thought this was idiomatic, but I will add a comment.

Rationale for inclusion of break over iteration over all one keys is https://godbolt.org/z/z95q8K77f.

belimawr · 2024-05-14T20:56:46Z

x-pack/filebeat/input/http_endpoint/handler.go

@@ -64,18 +69,40 @@ type handler struct {
 }

 func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	txID := h.nextTxID()
+	h.log.Debugw("request", "url", r.URL, "tx_id", txID)


I'm worried about debug logs in hot paths like that. If there is a high throughput of incoming requests and a user turns on debug logging, this can flood the logs.

It's not a blocker, I just wanted to raise my concern and let you decide what is best for this input.

I'm OK with this.

x-pack/filebeat/input/http_endpoint/handler.go

mergify · 2024-05-15T07:30:36Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 38979-http_endpoint upstream/38979-http_endpoint
git merge upstream/main
git push upstream 38979-http_endpoint

andrewkroh · 2024-05-17T01:01:42Z

x-pack/filebeat/docs/inputs/input-http-endpoint.asciidoc

+The endpoint will enforce end-to-end ACK when a URL query parameter
+`wait_for_completion_timeout` with a duration is provided. For example
+`http://localhost:8080/?wait_for_completion_timeout=1m` will wait up
+to 1min for the event to be published to the cluster and then return


Suggested change

to 1min for the event to be published to the cluster and then return

to 1 minute for the event to be published to the cluster and then return

andrewkroh · 2024-05-17T01:02:08Z

x-pack/filebeat/docs/inputs/input-http-endpoint.asciidoc

+`http://localhost:8080/?wait_for_completion_timeout=1m` will wait up
+to 1min for the event to be published to the cluster and then return
+the user-defined response message. In the case that the publication
+does not happen within the timeout duration, the HTTP response will


Suggested change

does not happen within the timeout duration, the HTTP response will

does not complete within the timeout duration, the HTTP response will

mergify · 2024-05-17T03:01:50Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b 38979-http_endpoint upstream/38979-http_endpoint
git merge upstream/main
git push upstream 38979-http_endpoint

…c root

The filename coming from integrations may include a * which is intended to be replaced with the data stream ID. The code in place does not do this, so add it.

…ames This is a partial backport of elastic#39410 to fix the missing replacement of "*" with the input ID.

…ames (#39656) This is a partial backport of #39410 to fix the missing replacement of "*" with the input ID.

efd6 self-assigned this May 6, 2024

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

efd6 force-pushed the 38979-http_endpoint branch from 5107bd5 to d9d3c36 Compare May 6, 2024 04:16

efd6 added Filebeat Filebeat backport-skip Skip notification from the automated backport with mergify Team:Security-Service Integrations Security Service Integrations Team enhancement and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 6, 2024

efd6 marked this pull request as ready for review May 8, 2024 00:38

efd6 requested review from a team as code owners May 8, 2024 00:38

efd6 requested review from AndersonQ and rdner May 8, 2024 00:38

efd6 marked this pull request as draft May 8, 2024 00:38

efd6 marked this pull request as ready for review May 8, 2024 04:10

efd6 force-pushed the 38979-http_endpoint branch from 4f5f26d to 1ee93be Compare May 8, 2024 08:32

andrewkroh reviewed May 8, 2024

View reviewed changes

efd6 force-pushed the 38979-http_endpoint branch from dbfa282 to 79d5b0a Compare May 8, 2024 22:43

efd6 force-pushed the 38979-http_endpoint branch 3 times, most recently from ef8bd51 to 6ec5edd Compare May 9, 2024 22:47

pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 10, 2024

efd6 requested a review from belimawr May 12, 2024 22:21

efd6 mentioned this pull request May 13, 2024

http_endpoint: test ack api elastic/integrations#9843

Merged

4 tasks

belimawr reviewed May 14, 2024

View reviewed changes

belimawr approved these changes May 14, 2024

View reviewed changes

efd6 force-pushed the 38979-http_endpoint branch from a606c9c to ed0850b Compare May 15, 2024 07:35

andrewkroh approved these changes May 17, 2024

View reviewed changes

efd6 force-pushed the 38979-http_endpoint branch from ed0850b to f4cab4c Compare May 17, 2024 01:22

andrewkroh approved these changes May 17, 2024

View reviewed changes

efd6 mentioned this pull request May 17, 2024

http_endpoint: provide request tracing support elastic/integrations#9909

Draft

4 tasks

efd6 added 13 commits May 17, 2024 12:31

x-pack/filebeat/input/http_endpoint: make input GA

338227a

x-pack/filebeat/input/http_endpoint: add acking

3d87bdf

address pr comments

8972f9e

x-pack/filebeat/input/http_endpoint: add e2e acking

f2748c1

x-pack/filebeat/input/http_endpoint: add debug URL logging

9bbcac2

x-pack/filebeat/docs/inputs: add timeout documentation

4e9325e

address pr comment

d08dad2

validate queries

310ce79

add system test for acking

db44a7b

x-pack/filebeat/input/http_endpoint: allow user to place events at do…

e901753

…c root

address pr comment

b7d7f50

address pr comments

19ce29d

x-pack/filebeat/input/http_endpoint: fix request trace filename handling

0d7f581

The filename coming from integrations may include a * which is intended to be replaced with the data stream ID. The code in place does not do this, so add it.

efd6 force-pushed the 38979-http_endpoint branch from 0825086 to 0d7f581 Compare May 17, 2024 03:02

efd6 merged commit 6979849 into elastic:main May 17, 2024
17 of 19 checks passed

efd6 added a commit to efd6/beats that referenced this pull request May 22, 2024

x-pack/filebeat/input/http_endpoint: fix request trace logging file n…

c139120

…ames This is a partial backport of elastic#39410 to fix the missing replacement of "*" with the input ID.

efd6 added a commit to efd6/beats that referenced this pull request May 22, 2024

x-pack/filebeat/input/http_endpoint: fix request trace logging file n…

944729f

…ames This is a partial backport of elastic#39410 to fix the missing replacement of "*" with the input ID.

efd6 mentioned this pull request May 22, 2024

x-pack/filebeat/input/http_endpoint: fix request trace logging file names #39656

Merged

6 tasks

efd6 added a commit that referenced this pull request May 22, 2024

x-pack/filebeat/input/http_endpoint: fix request trace logging file n…

883e6c3

…ames (#39656) This is a partial backport of #39410 to fix the missing replacement of "*" with the input ID.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x-pack/filebeat/input/http_endpoint: make input GA #39410

x-pack/filebeat/input/http_endpoint: make input GA #39410

efd6 commented May 6, 2024 •

edited

mergify bot commented May 6, 2024

elasticmachine commented May 6, 2024 •

edited

Build stats

elasticmachine commented May 8, 2024

andrewkroh May 8, 2024 •

edited

andrewkroh May 8, 2024

efd6 May 8, 2024

andrewkroh May 9, 2024

andrewkroh May 8, 2024

efd6 May 8, 2024

andrewkroh May 9, 2024

efd6 May 9, 2024

mergify bot commented May 8, 2024

mergify bot commented May 9, 2024

elasticmachine commented May 10, 2024

belimawr May 14, 2024

efd6 May 14, 2024 •

edited

belimawr May 14, 2024

efd6 May 14, 2024

mergify bot commented May 15, 2024

andrewkroh May 17, 2024

andrewkroh May 17, 2024

mergify bot commented May 17, 2024

	to 1min for the event to be published to the cluster and then return
	to 1 minute for the event to be published to the cluster and then return

	does not happen within the timeout duration, the HTTP response will
	does not complete within the timeout duration, the HTTP response will

x-pack/filebeat/input/http_endpoint: make input GA #39410

x-pack/filebeat/input/http_endpoint: make input GA #39410

Conversation

efd6 commented May 6, 2024 • edited

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented May 6, 2024

elasticmachine commented May 6, 2024 • edited

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

elasticmachine commented May 8, 2024

andrewkroh May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 8, 2024

mergify bot commented May 9, 2024

elasticmachine commented May 10, 2024

Choose a reason for hiding this comment

efd6 May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 17, 2024

efd6 commented May 6, 2024 •

edited

elasticmachine commented May 6, 2024 •

edited

andrewkroh May 8, 2024 •

edited

efd6 May 14, 2024 •

edited