pkg/util/log: change default httpSink timeout to 2s

[abarganier]

Previously, the default timeout for the httpSink was
for there to be no timeout at all.

This means that the first call to output() on where
the http target was unavailable would hang forever.
This would deadlock the calling goroutine, whether
that's the bufferedSink flush goroutine, or (even worse)
a server goroutine in the event that the httpSink is not
buffered.

Our default timeout should not deadlock in the worst case
scenario. Admittedly, 2s would also cause a noticeable
performance degradation in the event that the httpSink was
unbuffered, but it would at least be able to emit logs
indicating the timeout as the cause. Availability would also
be maintained to some degree. Previously, the deadlocks
due to no timeout being set by default meant that no
indication was ever given that the httpSink was unable
to reach the http target.

Release note (ops change): The default value of timeout
for http-servers logging sinks has been changed from
"no timeout" to 2s. This will be reflected in the
http-defaults section of the log configuration. Users
still maintain the ability to override the timeout, or
disable it by explicitly setting it to 0 (e.g. timeout: 0).

Fixes: #109263

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[abarganier]

cc @florence-crl - this will require a docs change to update the http-defaults/timeout default value in our log config docs: https://www.cockroachlabs.com/docs/stable/configure-logs#output-to-http-network-collectors

[dhartunian]

do we have same thing with fluent-servers?

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

[abarganier]

TFTR!

fluent-servers don't have timeout config options, but they do have hardcoded dial and write timeouts (5 & 1 seconds, respectively):

cockroach/pkg/util/log/fluent_client.go

Lines 38 to 39 in abd9c99

	const fluentDialTimeout = 5 * time.Second
	const fluentWriteTimeout = time.Second

This makes me wonder - should the default http timeout be more aggressive than 60s? Maybe 5s?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @knz)

[abarganier]

Based on some internal discussion, I've dropped the default from 60s to 2s. This aligns with our general network timeout.

[sean-]

Why align with our network timeouts vs max(${network_timeout}, ${storage_timeout})? EBS has a 60s IO timeout for storage. Is the sink synchronous or an async, buffered leaky bucket? And if it is a leaky bucket, do we have any metrics around dropped messages when the bucket gets full, and we start discarding messages?

[abarganier]

Why align with our network timeouts vs max(${network_timeout}, ${storage_timeout})? EBS has a 60s IO timeout for storage. Is the sink synchronous or an async, buffered leaky bucket? And if it is a leaky bucket, do we have any metrics around dropped messages when the bucket gets full, and we start discarding messages?

See discussion here: https://cockroachlabs.slack.com/archives/C01CDD4HRC5/p1692804661841219

We don't have precedent for setting conditional defaults for log configs, AFAIK (e.g. using a max function).

The async buffer (if enabled, which it is by default, but can be disabled) is leaky once a limit is reached. Regarding metrics for this, see #72453. The infra wasn't there to record this until quite recently, see #106607.

[abarganier]

bors r=dhartunian

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)