sql: fix exporting of events to Obs Service#87054
sql: fix exporting of events to Obs Service#87054craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
In cockroachdb#86174, exporting of events to the Obs Service was doubly-decoupled from the SQL statement causing the event: it ran in an async task inside another async task. The inner task was given a context canceled when the outer task finished - thus introducing a race causing the event to not be passed along. This was caught by TestEventIngestionIntegration flakiness. The second task was unnecessary. Its purpose was to shield the caller from slow communication with the Obs Service, but the client library being used already does that (it's non-blocking). Release note: None
|
This change is |
abarganier
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @knz)
pkg/sql/event_log.go line 616 at r1 (raw file):
query, args, otelEvents := prepareEventWrite(ctx, execCfg, entries) // Send to the Obs Service.
nit: is it worth noting in a comment here that we don't need to worry about a "clogged" output queue to obsservice, since records are dropped?
Code quote:
// Send to the Obs Service.
andreimatei
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/sql/event_log.go line 616 at r1 (raw file):
Previously, abarganier (Alex Barganier) wrote…
nit: is it worth noting in a comment here that we don't need to worry about a "clogged" output queue to obsservice, since records are dropped?
I've made a note on the lower-level method that it's non-blocking. I think that's close enough to all the callers, so they don't have to repeat it.
|
Build succeeded: |
In #86174, exporting of events to the Obs Service was doubly-decoupled
from the SQL statement causing the event: it ran in an async task inside
another async task. The inner task was given a context canceled when the
outer task finished - thus introducing a race causing the event to not
be passed along. This was caught by TestEventIngestionIntegration
flakiness.
The second task was unnecessary. Its purpose was to shield the caller
from slow communication with the Obs Service, but the client library
being used already does that (it's non-blocking).
Release note: None
Release justification: bug fix, test flakiness