Add two client-side warnings to detect non-optimal use of the SDKs#330
Add two client-side warnings to detect non-optimal use of the SDKs#330irinatomic-db wants to merge 1 commit into
Conversation
irinatomic-db
force-pushed
the
irina-tomic_data/irinatomic-db/add-warnings
branch
3 times, most recently
from
5825e93 to
0e927bf
Compare
| ) | ||
| })??); | ||
|
|
||
| client_warnings::record_stream_opened(&table_properties.table_name); |
There was a problem hiding this comment.
Both new and recreate_stream hit this path. If I am reading this correctly, we do double counting during recreate.
Ideally, recreate stream shouldn't count as churn.
There was a problem hiding this comment.
Fixed. Moved the call out of new_stream() and into the builder, so recreate_stream and recreate_arrow_stream no longer count as churn.
| ### New Features and Improvements | ||
|
|
||
| - Added process-wide client-side warnings to surface common misuse patterns: | ||
| - **Concurrent open streams**: logs a `WARN` when 32 or more ingest streams for the same table are open simultaneously. |
There was a problem hiding this comment.
Do we really need both patterns? They are kind of doing a similar thing.
Can we just have 100 per 60 seconds?
What is the scenario where we care about 32 concurrent streams?
There was a problem hiding this comment.
Good point.
Main idea was to check if someone is misusing the SDK (treating it like a file push system, opening 1 stream for each record). But the stream churn warning is enough. I'll keep only that one.
| /// message the first time the count within the window reaches | ||
| /// [`CHURN_WARN_THRESHOLD`] (100). The warning re-fires if the rate drops below | ||
| /// the threshold and later surges again in a new window. | ||
| pub(crate) fn record_stream_opened(table_name: &str) { |
There was a problem hiding this comment.
Can we do better naming? register_stream_opened and record_stream_opened sound the same.
(ideally, let's just keep only churn_threshold = 100 so we don't need to do both.
There was a problem hiding this comment.
Dropped the concurrent monitor entirely
irinatomic-db
force-pushed
the
irina-tomic_data/irinatomic-db/add-warnings
branch
from
0e927bf to
f00f9a5
Compare
| } | ||
| ``` | ||
|
|
||
| ## Client-side warnings |
There was a problem hiding this comment.
We should add this entry in table of contents
|
Btw, you need to setup signed commits. Since you've already pushed commits, it needs to be done "retroactively". I usually do it like this: |
irinatomic-db
force-pushed
the
irina-tomic_data/irinatomic-db/add-warnings
branch
from
f00f9a5 to
5d1e988
Compare
3 participants
What changes are proposed in this pull request?
Adds two process-wide client-side warnings to
ZerobusStreamandZerobusArrowStreamthat surface common stream misuse patterns at runtime.Stream churn - fires a
WARNwhen 100 or more streams for the same table are opened within a 60-second sliding window. Re-fires after the rate recovers and rises again.Both warnings live in a new
client_warningsmodule and can be suppressed withZEROBUS_SDK_WARNINGS_ENABLED=false.Why: Users sometimes accidentally create a new stream per record instead of reusing streams, or fan out to many concurrent streams unnecessarily. These patterns cause undue load on the service and are hard to spot from the outside. Python and TypeScript pick these up automatically; Go and Java will on the next FFI release.
How is this tested?
Added unit tests.
For manual verification, a temporary test was added that initialised a tracing subscriber and opened streams in a loop to confirm both
WARNlines fired correctly. It was removed before this PR.