SNOW-4008939 sf_core: keep the telemetry raw-log message a JSON object so message:data stays readable - #1343
Closed
sfc-gh-merbel wants to merge 1 commit into
Closed
Conversation
`TelemetrySendLog` takes `message_json` as a string, and the raw-log lane parsed it into any `serde_json::Value` before embedding it as the envelope's `message`. Nothing required an object, so a caller sending a scalar or an array parsed successfully and produced an envelope with a non-object message. Consumers read payload fields as `message:data:<field>` against a VARIANT. Against a scalar or array that reads NULL rather than failing, so such a caller creates rows that look present but carry no readable payload, and nothing on either side reports a problem. Normalise instead of rejecting: a non-object parse is nested under `message` rather than dropped, which keeps the envelope shape predictable while honouring this lane's existing rule that one bad entry must neither sink the batch nor silently vanish. The already-handled malformed-JSON case produced a JSON string and so was affected by the same problem; it now takes the same path, which is the one behaviour change visible to an existing caller. Objects — every real caller — are passed through untouched, so well-behaved field paths are unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Closing: this was opened against the wrong repository. Universal Driver work belongs in the EMU org, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes the raw-log lane in
sf_core/src/telemetry/session_telemetry.rsso that themessagefield of a/telemetry/sendlog entry is always a JSON object.RawLogEntry::into_log_entriespreviously parsed the caller'smessage_jsoninto anyserde_json::Valueand embedded whatever came out; it now passes objects through untouched and nests anything else under amessagekey. No API, wire-envelope or proto change is involved, and for every caller that already sends an object — which is all of them today — the emitted payload is byte-identical.The motivation is SNOW-4008939.
TelemetrySendLogacceptsmessage_jsonas astring, and nothing required the parsed value to be an object, so a caller sending a scalar or an array parsed successfully and produced an envelope with a non-objectmessage. That matters because downstream consumers read payload fields asmessage:data:<field>against a VARIANT: against a scalar or an array, that expression yields NULL rather than an error. A misbehaving producer therefore creates rows that look present but carry no readable payload, and nothing on either side reports a problem. The severity is low — it takes a misbehaving caller, and the damage is confined to that caller's own rows — but the failure is silent in both directions, which makes it far cheaper to close now than to diagnose from a dashboard later.Of the two fixes the ticket suggested, rejecting the entry or wrapping it, this takes the wrapping route. This lane already has a stated rule for bad input, in the doc comment on the function being changed: one bad entry must never sink the whole batch, but it also should not silently vanish. Rejecting would satisfy the first half and violate the second, and it would do so without any way to tell the caller, since
telemetry_send_loghas no error channel back to the wrapper — it returns an emptyTelemetrySendResponseunconditionally. Wrapping keeps the payload, makes the envelope shape predictable, and leaves the well-behaved path allocation-for-allocation identical. Worth flagging for review: the malformed-JSON case, which the ticket treated as already handled, producedValue::String(raw)and so was affected by exactly the same problem — a JSON string is not an object either. It now takes the same wrapping path, and that is the one behaviour change an existing caller could observe. The two cases are logged distinguishably at debug level so a misbehaving producer can still be identified.The risk is small and concentrated in that one behaviour change. There is no Java or JDBC caller of this RPC today; the wired consumers are the Rust core and Python, and neither sends a non-object message, so nothing shipping changes shape. Objects gain no extra nesting level, so existing field paths are untouched. Anyone who was relying on a malformed entry arriving as a bare string would now find it at
message.message, which is a deliberate trade: that reading was already unusable through the documentedmessage:dataaccess path. The change adds no allocation on the object path and no new dependency.Testing is at the unit level, alongside the existing cases in the same module.
raw_log_entry_nests_non_object_json_under_messagecovers a number, an array, a string, a boolean andnull, asserting both thatmessageis an object and that the original value survives atmessage.message.raw_log_entry_does_not_nest_an_object_messagepins the no-regression case, asserting that an object gains no wrapper key. The pre-existing malformed-JSON test was updated to the new expectation rather than deleted, so the fallback stays covered. All 12 unit tests intelemetry::session_telemetrypass, as do all 35 tests in thetelemetryintegration module, which exercise the real wire body through wiremock and would have caught an envelope-shape regression.cargo fmtandcargo clippy -p sf_core --libare both clean, the only clippy output being a pre-existing deprecation warning elsewhere in the crate.One note for the reviewer on provenance: the ticket cites
origin/mainat4846f3466, which is not an ancestor ofmainand could not be resolved in this clone, so this work is based onaac98014binstead. The code in question is unchanged in substance between the two descriptions. Separately, the ticket records two adjacent behaviours that were investigated and deliberately not changed here — key reordering fromserde_json'sBTreeMapbacking, which is harmless because VARIANT field access is order-insensitive, and integer precision, where an earlier claim of degradation above 2^53 was measured and found to be wrong. A related but independent issue in this lane, the absence of any per-call flush, is tracked separately as SNOW-4008940 and is not touched by this PR.Made with Cursor