Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: allow registering a structured event listener on a tracing span #80395

Closed
adityamaru opened this issue Apr 22, 2022 · 1 comment
Closed
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@adityamaru
Copy link
Contributor

adityamaru commented Apr 22, 2022

A tracing span should expose a listener API that can be used to listen in on structured events that are recorded by the span and its children. This is motivated by the need for higher-level aggregators such as #80388 to be able to aggregate information from interesting events that are recorded throughout a root spans lifetime. An event listener allows us to be certain that we won't miss events that are rotated out of the underlying ring buffer due to memory constraints. Jobs for example can run for hours and emit thousands of structured events, we want to be able to account for every event in our metrics to power our observability tools accurately.

Epic: CRDB-10262

Jira issue: CRDB-15626

@adityamaru adityamaru added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Apr 22, 2022
@adityamaru adityamaru self-assigned this Apr 25, 2022
adityamaru added a commit to adityamaru/cockroach that referenced this issue Apr 25, 2022
This change extends the tracing API to support registering
and unregistering an EventListener on a span. The registered
EventListener will be notified about every StructuredEvent
recorded by the span. EventListeners registered with a span
are inherited by the span's local children.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Note, event listeners can only be registered, and inherited by spans
with a RecordingStructured or RecordingVerbose recording type.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue Apr 25, 2022
This change extends the tracing API to support registering
and unregistering an EventListener on a span. The registered
EventListener will be notified about every StructuredEvent
recorded by the span. EventListeners registered with a span
are inherited by the span's local children.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Note, event listeners can only be registered, and inherited by spans
with a RecordingStructured or RecordingVerbose recording type.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 1, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 1, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 2, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 14, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 16, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
adityamaru added a commit to adityamaru/cockroach that referenced this issue May 16, 2022
This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: cockroachdb#80395

Release note: None
craig bot pushed a commit that referenced this issue May 16, 2022
80460: tracing: introduce a StructuredEvent listener r=andreimatei a=adityamaru

This change adds an option `WithEventListeners(...)` that allows for
specifying an EventListener during span creation. An EventListener will
be notified on every StructuredEvent recorded by the span and its children.
The event listeners will be removed once the span is Finish()'ed and will no
longer receive StructuredEvent notifications. Remote spans will notify
the registerd event listeners when the remote span recording is imported into
the span with the event listeners.

The motivation for this change was to allow higher level aggregators
to watch every event without relying on periodically pulling their
root span's Recording. This way the aggregator can be sure not to
miss "important" events that may be rotated out of the underlying
ring buffer due to memory constraints.

Informs: #80395

Release note: None

81225: log: make fluentSink thread safe r=knz a=dhartunian

Previously, the fluentSink implementation was not thread-safe. On
configurations with multiple `loggerT` instances the fluentSink
interface would be shared between them and could cause panics at
runtime.

This commit adds a mutex to the fluentSink implementation that is locked
during output.

NB: httpSink is already thread safe by construction and interceptorSink
already has a mutex that manages concurrent access.

Resolves #81112

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
@jlinder jlinder added sync-me and removed sync-me labels May 20, 2022
@adityamaru
Copy link
Contributor Author

Closed by #80460.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

2 participants