ehancement(sources): Add `keepalive.max_connection_age_secs` config option to HTTP-server sources by dsmith3197 · Pull Request #19141 · vectordotdev/vector

dsmith3197 · 2023-11-13T22:13:44Z

Clients often use indefinitely persistent connections. Given that we recommend network-level load balancers, this can result in sub-par balancing as the worker pool grows due to a lack of rebalancing.

To resolve this, sources with a HTTP server now support a configurable option to send a Connection: close header and close the connection after a certain time interval.

The default max age is 5 minutes.

github-actions · 2023-11-13T22:14:35Z

Your preview site for the VRL Playground will be ready in a few minutes, please allow time for it to build.

Heres your preview link:
VRL Playground preview

github-actions · 2023-11-13T22:14:37Z

Your preview site for the vector.dev will be ready in a few minutes, please allow time for it to build.

Heres your preview link:
vector.dev preview

github-actions · 2023-11-13T22:14:37Z

Your preview site for the Rust Doc will be ready in a few minutes, please allow time for it to build.

Heres your preview link:
Rust Doc preview

jszwedko

Awesome! This is a nice addition on its own, but I think we discussed that Vector should close the connection, itself, after sending the response with this header. Do you still plan to implement that?

Additionally, yes, I was thinking about making this a default for at least the Datadog Agent source.

I'd like to have someone else do a more detailed review (maybe @bruceg?). My review was more high-level and focused on UX.

jszwedko · 2023-11-13T23:28:01Z

            Some(&HeaderValue::from_static("foo"))
        );
    }
+


Nice tests 👍

dsmith3197 · 2023-11-13T23:48:37Z

Awesome! This is a nice addition on its own, but I think we discussed that Vector should close the connection, itself, after sending the response with this header. Do you still plan to implement that?

Correct HTTP client implementations will close the connection when they receive the Connection: close. I validated this in the tests for the hyper client and confirmed this is true for the go http client as well. With that in mind, I think closing the connection is unnecessary and not worth the additional coordination / complexity required. It's not clear to me how we would safely close the connection after ensuring delivery of the current response. What do you think?

dsmith3197 · 2023-11-14T13:54:02Z

Awesome! This is a nice addition on its own, but I think we discussed that Vector should close the connection, itself, after sending the response with this header. Do you still plan to implement that?

Correct HTTP client implementations will close the connection when they receive the Connection: close. I validated this in the tests for the hyper client and confirmed this is true for the go http client as well. With that in mind, I think closing the connection is unnecessary and not worth the additional coordination / complexity required. It's not clear to me how we would safely close the connection after ensuring delivery of the current response. What do you think?

I looked into this a bit more. The hyper server actually takes care of gracefully closing the connection for us as well, so there's nothing else we have to do here 🙂

https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/role.rs#L798
https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/conn.rs#L524

datadog-vectordotdev · 2023-11-14T14:42:51Z

Datadog Report

Branch report: dougsmith/OPW-136
Commit report: 02ea1e0

✅ vector: 0 Failed, 0 New Flaky, 2207 Passed, 0 Skipped, 25m 5.99s Wall Time

bruceg

The code looks reasonable but I have a couple of questions ~~and a doubt about the term "jitter"~~.

bruceg · 2023-11-14T18:11:37Z

+    start_reference: Instant,
+    max_connection_age: Duration,


Would it be worth storing the end reference instead of the start plus an age? i.e. store Instant::now() + max_connection_age

The start reference and age are useful information to put in the debug log in my opinion.

Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com>

jszwedko · 2023-11-14T20:19:24Z

I looked into this a bit more. The hyper server actually takes care of gracefully closing the connection for us as well, so there's nothing else we have to do here 🙂

https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/role.rs#L798 https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/conn.rs#L524

Aha, nice! It might be worth testing that behavior, if it is easy, since it would not be obvious to me that hyper has this behavior and there is a, admittedly small, risk that we could regress if we ever swapped the HTTP client implementation. I see you already added a comment which I was also going to recommend 😄

dsmith3197 · 2023-11-14T20:31:42Z

I looked into this a bit more. The hyper server actually takes care of gracefully closing the connection for us as well, so there's nothing else we have to do here 🙂
https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/role.rs#L798 https://github.com/hyperium/hyper/blob/6fd696e10974f10b2c6b9d16393bbbfa21c2333f/src/proto/h1/conn.rs#L524

Aha, nice! It might be worth testing that behavior, if it is easy, since it would not be obvious to me that hyper has this behavior and there is a, admittedly small, risk that we could regress if we ever swapped the HTTP client implementation. I see you already added a comment which I was also going to recommend 😄

The following test indirectly asserts that the connection is reset by asserting that the Connection: close header is absent on subsequent requests. Now, albeit the connection may be closed in the client or server because they both have logic to do so. I don't see an easy way to assert that the underlying tcp connection is closed specifically by the server.

https://github.com/vectordotdev/vector/pull/19141/files#diff-0820c0d03ae75b35f0b5bbd8dbc8e4d7ca048d19d60351bef069ac00c236910dR754

With that being said, it would good to add an integration test that verifies this behavior as well. I can look into that in a follow-up PR.

github-actions · 2023-11-15T21:41:54Z

Regression Detector Results

Run ID: b70e9a32-91cb-4d9e-b8e4-212c80c28365
Baseline: c023196
Comparison: 59e6d36
Total vector CPUs: 7

Explanation

A regression test is an integrated performance test for vector in a repeatable rig, with varying configuration for vector. What follows is a statistical summary of a brief vector run for each configuration across SHAs given above. The goal of these tests are to determine quickly if vector performance is changed and to what degree by a pull request.

Because a target's optimization goal performance in each experiment will vary somewhat each time it is run, we can only estimate mean differences in optimization goal relative to the baseline target. We express these differences as a percentage change relative to the baseline target, denoted "Δ mean %". These estimates are made to a precision that balances accuracy and cost control. We represent this precision as a 90.00% confidence interval denoted "Δ mean % CI": there is a 90.00% chance that the true value of "Δ mean %" is in that interval.

We decide whether a change in performance is a "regression" -- a change worth investigating further -- if both of the following two criteria are true:

The estimated |Δ mean %| ≥ 5.00%. This criterion intends to answer the question "Does the estimated change in mean optimization goal performance have a meaningful impact on your customers?". We assume that when |Δ mean %| < 5.00%, the impact on your customers is not meaningful. We also assume that a performance change in optimization goal is worth investigating whether it is an increase or decrease, so long as the magnitude of the change is sufficiently large.
Zero is not in the 90.00% confidence interval "Δ mean % CI" about "Δ mean %". This statement is equivalent to saying that there is at least a 90.00% chance that the mean difference in optimization goal is not zero. This criterion intends to answer the question, "Is there a statistically significant difference in mean optimization goal performance?". It also means there is no more than a 10.00% chance this criterion reports a statistically significant difference when the true difference in mean optimization goal is zero -- a "false positive". We assume you are willing to accept a 10.00% chance of inaccurately detecting a change in performance when no true difference exists.

The table below, if present, lists those experiments that have experienced a statistically significant change in mean optimization goal performance between baseline and comparison SHAs with 90.00% confidence OR have been detected as newly erratic. Negative values of "Δ mean %" mean that baseline is faster, whereas positive values of "Δ mean %" mean that comparison is faster. Results that do not exhibit more than a ±5.00% change in their mean optimization goal are discarded. An experiment is erratic if its coefficient of variation is greater than 0.1. The abbreviated table will be omitted if no interesting change is observed.

No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%.

Fine details of change detection per experiment.

experiment	goal	Δ mean %	Δ mean % CI	confidence
syslog_log2metric_splunk_hec_metrics	ingress throughput	+4.17	[+4.03, +4.31]	100.00%
syslog_humio_logs	ingress throughput	+3.24	[+3.16, +3.32]	100.00%
syslog_loki	ingress throughput	+3.10	[+3.07, +3.14]	100.00%
syslog_splunk_hec_logs	ingress throughput	+2.82	[+2.79, +2.86]	100.00%
otlp_grpc_to_blackhole	ingress throughput	+1.35	[+1.25, +1.44]	100.00%
http_text_to_http_json	ingress throughput	+1.35	[+1.22, +1.47]	100.00%
syslog_regex_logs2metric_ddmetrics	ingress throughput	+1.31	[+1.23, +1.39]	100.00%
socket_to_socket_blackhole	ingress throughput	+1.23	[+1.15, +1.30]	100.00%
datadog_agent_remap_datadog_logs	ingress throughput	+0.84	[+0.75, +0.93]	100.00%
datadog_agent_remap_blackhole_acks	ingress throughput	+0.82	[+0.73, +0.92]	100.00%
fluent_elasticsearch	ingress throughput	+0.53	[+0.08, +0.98]	94.55%
otlp_http_to_blackhole	ingress throughput	+0.38	[+0.22, +0.53]	99.99%
http_to_s3	ingress throughput	+0.30	[+0.03, +0.58]	93.18%
http_to_http_noack	ingress throughput	+0.11	[+0.02, +0.20]	94.79%
http_to_http_json	ingress throughput	+0.03	[-0.04, +0.11]	55.56%
datadog_agent_remap_blackhole	ingress throughput	+0.01	[-0.07, +0.09]	22.64%
splunk_hec_to_splunk_hec_logs_acks	ingress throughput	-0.00	[-0.14, +0.14]	0.02%
splunk_hec_indexer_ack_blackhole	ingress throughput	-0.00	[-0.14, +0.13]	2.31%
splunk_hec_to_splunk_hec_logs_noack	ingress throughput	-0.05	[-0.17, +0.06]	55.48%
datadog_agent_remap_datadog_logs_acks	ingress throughput	-0.10	[-0.18, -0.03]	97.36%
enterprise_http_to_http	ingress throughput	-0.11	[-0.19, -0.03]	97.26%
splunk_hec_route_s3	ingress throughput	-0.60	[-1.11, -0.09]	94.64%
http_elasticsearch	ingress throughput	-0.71	[-0.78, -0.65]	100.00%
http_to_http_acks	ingress throughput	-0.99	[-2.30, +0.31]	79.08%
file_to_blackhole	egress throughput	-1.43	[-3.90, +1.04]	65.98%
syslog_log2metric_humio_metrics	ingress throughput	-2.41	[-2.52, -2.30]	100.00%

…ption to HTTP-server sources (#19141) * OPW-136 * add docs * clippy * update comments * fmt * Update src/http.rs Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com> * proptest * remove sentinel value * add upgrade guide entry * spelling --------- Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com>

…ption to HTTP-server sources (vectordotdev#19141) * OPW-136 * add docs * clippy * update comments * fmt * Update src/http.rs Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com> * proptest * remove sentinel value * add upgrade guide entry * spelling --------- Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com>

OPW-136

12a1ba5

github-actions Bot added the domain: sources Anything related to the Vector's sources label Nov 13, 2023

github-advanced-security AI found potential problems Nov 13, 2023

View reviewed changes

Comment thread src/http.rs Fixed

Comment thread src/http.rs Fixed

add docs

ec4eb8d

github-actions Bot added the domain: external docs Anything related to Vector's external, public documentation label Nov 13, 2023

dsmith3197 marked this pull request as ready for review November 13, 2023 22:27

dsmith3197 requested a review from a team November 13, 2023 22:27

dsmith3197 requested review from a team and StephenWakely as code owners November 13, 2023 22:27

dsmith3197 requested a review from a team November 13, 2023 22:27

dsmith3197 requested a review from neuronull as a code owner November 13, 2023 22:27

brett0000FF approved these changes Nov 13, 2023

View reviewed changes

jszwedko reviewed Nov 13, 2023

View reviewed changes

dsmith3197 requested a review from bruceg November 14, 2023 13:27

clippy

c638b08

dsmith3197 added 2 commits November 14, 2023 08:57

update comments

955efc7

fmt

0f88b39

bruceg reviewed Nov 14, 2023

View reviewed changes

dsmith3197 and others added 2 commits November 14, 2023 14:11

Update src/http.rs

1723aee

Co-authored-by: Bruce Guenter <bruce.guenter@datadoghq.com>

proptest

38d5776

dsmith3197 requested a review from bruceg November 14, 2023 19:36

jszwedko approved these changes Nov 14, 2023

View reviewed changes

dsmith3197 added 3 commits November 14, 2023 15:56

remove sentinel value

7481fb6

add upgrade guide entry

56804df

spelling

3ade87e

jszwedko added this pull request to the merge queue Nov 15, 2023

Merged via the queue into master with commit 59e6d36 Nov 15, 2023

jszwedko deleted the dougsmith/OPW-136 branch November 15, 2023 22:06

dsmith3197 mentioned this pull request Dec 22, 2023

Add max connection age settings to gRPC servers for improved load balancing #19457

Closed

Uh oh!

Conversation

dsmith3197 commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 13, 2023

Uh oh!

github-actions Bot commented Nov 13, 2023

Uh oh!

github-actions Bot commented Nov 13, 2023

Uh oh!

Uh oh!

Uh oh!

jszwedko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jszwedko Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsmith3197 commented Nov 13, 2023

Uh oh!

dsmith3197 commented Nov 14, 2023

Uh oh!

datadog-vectordotdev Bot commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

bruceg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bruceg Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

dsmith3197 Nov 14, 2023

Choose a reason for hiding this comment

Uh oh!

jszwedko commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsmith3197 commented Nov 14, 2023

Uh oh!

github-actions Bot commented Nov 15, 2023

Regression Detector Results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dsmith3197 commented Nov 13, 2023 •

edited

Loading

jszwedko left a comment •

edited

Loading

datadog-vectordotdev Bot commented Nov 14, 2023 •

edited

Loading

bruceg left a comment •

edited

Loading

jszwedko commented Nov 14, 2023 •

edited

Loading