I'd be curious to see if the StartSpanCtx numbers improve if we remove eventListenerAlloc.

And can you please tell me what the two new allocations in RecordingWithStructuredEvent are?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

pkg/util/tracing/crdbspan.go line 131 at r1 (raw file):

	// whenever a Structured event is recorded by the span.
	//
	// Child spans of this span will inherit this list of EventListeners.

s/of this span/

pkg/util/tracing/crdbspan.go line 141 at r1 (raw file):

type eventListener struct {
	Key      string

do the fields need to be exported? Unexported struct with exported fields is weird, I think.

pkg/util/tracing/crdbspan.go line 555 at r1 (raw file):

	}

	// TODO(during review): Should we limit the number of listeners a span can register?

I wouldn't worry about it

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

// unregisterEventListenerLocked unregisters a previously registered
// EventListener.
func (s *crdbSpan) unregisterEventListenerLocked(key string) {

do you think we need deregistration? And also the de-duping by key? What use cases do you have in mind?

pkg/util/tracing/crdbspan.go line 587 at r1 (raw file):

	for _, listener := range s.mu.eventListeners {
		wg.Add(1)
		go func(l EventListener) {

why call on a different goroutine, particularly since you keep the mutex for the duration? I pretty sure the overhead of the goroutines will be more than the theoretical parallelization.

pkg/util/tracing/span.go line 485 at r1 (raw file):

	// span recording Structured events during traced operations.
	//
	// Listen can be called concurrently with other Listen calls on the same

I'd either say more or less here. Say that the listener cannot be called concurrently on the same span, and talk about different spans.

pkg/util/tracing/span.go line 487 at r1 (raw file):

	// Listen can be called concurrently with other Listen calls on the same
	// EventListener.
	Listen(event Structured)

I think Notify or Signal would be more common names.

pkg/util/tracing/span.go line 490 at r1 (raw file):

}

// RegisterEventListener adds an EventListener to the span. The listener can

s/can listen/is notified

pkg/util/tracing/span.go line 490 at r1 (raw file):

}

// RegisterEventListener adds an EventListener to the span. The listener can

Let's talk about the relationship, or lack of, with recordings imported from remote spans. Do we call the listener on events from those at import time? (I think we should).

pkg/util/tracing/span.go line 499 at r1 (raw file):

}

// UnregisterEventListener unregisters a previously registered EventListener

if we keep the deregistration, let's specify that, once this returns, no further calls to Listen coming from this span are possible (and so commit to Unregister synchronizing with notifyEventListeners).
But what about children spans? Don't we want a recursive deregstration?
Or again lets consider if deregistration is really necessary.

pkg/util/tracing/span_options.go line 108 at r1 (raw file):

		opts.Parent.i.crdb.mu.Lock()
		defer opts.Parent.i.crdb.mu.Unlock()
		return opts.Parent.i.crdb.mu.eventListeners

I think we need to make a copy here instead of aliasing the parent slice (i.e. make the copy here and not in reset()).
Is it feasible to add a test for this bug? Like, create a parent, add a listener and then race an operation on the parent that touches that listener (e.g. a deregistration) with creating a child creation. Hopefully the race detector will catch it.

pkg/util/tracing/tracer.go line 1035 at r1 (raw file):

	h.childrenAlloc = [4]childRef{}
	h.structuredEventsAlloc = [3]interface{}{}
	h.eventListenerAlloc = [3]eventListener{}

I wouldn't bother with pooling listener slices until it becomes common for spans to have listeners (if ever).

pkg/util/tracing/bench_test.go line 83 at r2 (raw file):

	}

	// Benchmarks with a root and child span.

I find the old structure easier to read because it lists all the benchmarks. I'd rather add a listener parameter to tc (and then don't use unkeyed literals when constructing the array of tests since there'll be too many bools). And this way you can keep the names of the pre-existing benchmarks as they were.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dt)

pkg/util/tracing/crdbspan.go line 131 at r1 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 141 at r1 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 555 at r1 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

do you think we need deregistration?

I mentioned this below, but I don't think we do. I was worried about aggregators outliving the span they were listening on, but it makes more sense to establish a contract that calls out that behaviour as wrong.

Following the above, there is also no need for a key.

pkg/util/tracing/crdbspan.go line 587 at r1 (raw file):

makes sense, removed.

pkg/util/tracing/span.go line 485 at r1 (raw file):

Done.

pkg/util/tracing/span.go line 487 at r1 (raw file):

changed to Notify

pkg/util/tracing/span.go line 490 at r1 (raw file):

Done.

pkg/util/tracing/span.go line 490 at r1 (raw file):

good catch, added to the comment, and also added logic to notify the listener at the time of importing.

pkg/util/tracing/span.go line 499 at r1 (raw file):

But what about children spans? Don't we want a recursive deregstration?

That's a good point and convinces me that maybe we shouldn't have unregistration. My concern initially was the span outliving the object listening in on events, but I think we can rely on listeners to ensure this is not the case.

pkg/util/tracing/span_options.go line 108 at r1 (raw file):

oops yeah, you're right. I'll try working on a test.

pkg/util/tracing/tracer.go line 1035 at r1 (raw file):

Done.

pkg/util/tracing/bench_test.go line 83 at r2 (raw file):

Makes sense, done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, and @dt)

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

I think we should remove

well, we want the the listener to outlive the span, don't we? One thing I would do is remove the listeners

pkg/util/tracing/span.go line 499 at r1 (raw file):

I think the Listener should indeed outlive its span. But, I think that once the span is Finish()ed, the Listener shouldn't get any more events (i.e. from the children spans).
I think the way I'd implement it is different than what we currently have: instead of having each span independently have references to its listeners (direct and inherited), I think it might be better if we have spans route their structured events to their parent. We don't want to always do that, for performance, but I think we can do it for all spans that are children (direct on indirect) from a span with a listener. So, I'd have each span maintain a bit saying whether the parent is interested in the events. WDYT?

pkg/util/tracing/span.go line 478 at r3 (raw file):

// EventListener is an object that can be registered to listen for Structured
// events recorded by the span.

nit: s/by the span/by a span and its children

pkg/util/tracing/span.go line 480 at r3 (raw file):

// events recorded by the span.
type EventListener interface {
	// Notify is invoked on every Structured event recorded by the span.

s/by the span/by the span and its hildren, recursively.

pkg/util/tracing/span.go line 492 at r3 (raw file):

// RegisterEventListener adds an EventListener to the span. The listener is
// notified of Structured events recorded by the span and its children.

This says "and its children", but that's not true for the existing children, is it? As written, the listener only applies to future children. I don't think these are the semantics we want; we don't have such asymmetries in other parts of the API (e.g. enabling recording applies to both current and future children). So, if this API stays, I think we definitely want it to apply to the current children.
But, I'd consider if this API is needed. An alternative is to pass the Listener at span creation time as an option - WithListener or such. I think we should definitely do that even if this API stays. But, if we have the creation option, maybe we don't need the dynamic flavor, or at least not initially?
Also see next comment below.

pkg/util/tracing/span_options.go line 108 at r1 (raw file):

we're still not making a copy :P

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, and @dt)

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

Ahh i misspoke, I meant what I wrote below i.e "span outliving the object listening in on events".

pkg/util/tracing/span.go line 499 at r1 (raw file):

I think it might be better if we have spans route their structured events to their parent

I agree with your point about not supporting dynamic registration for the time being, and only relying on WithEventListeners() passed in on span creation. Assuming we only support WithEventListeners(), when a span records a structured event it'll do something like this? This way it recursively notifies the listeners registered with its parents (direct or indirect).

func (s *span) notifyListeners(item Structured) {
    if s.Parent != nil && s.Parent.RouteToNotifyListeners {
          s.Parent.notifyListeners(item)
     }

     for _, l := range s.listeners {
          l.Notify(item)
     }
}

Remote children will continue to work the same way they do in this PR, i.e. we notify on ImportRemoteSpans.

A span can always outlive its parent. The fact that lifetime of the child is unrelated to the lifetime of the parent can be expressed explicitly when the child is created through the WithFollowsFrom() option. Our tracer ignores that option; only 3rd party OpenTelemetry ones sometimes make use of it. Explicit or not, it's not clear what not allowing a child to outlive the parent would mean. Should uses of a child after the span is finished be considered a use-after-finish (which is a crash in test builds)? Or should the uses of that span be silently ignored? We don't take either of these options, and I think it's for the better.

if this is a problem since we'll remove the listeners from a span on span.Finish()?

I don't think it's a problem, provided that we do things as I was suggesting: have listeners be attached only to a single span, not also attached directly to the parent. The semantics are then clear: you attach a listener to a span and, once that span finishes, the listener will not be notified any more. I think that'd be a good API.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

my comment here was incomplete/gibberish, as I had moved my real comments elsewhere :)

pkg/util/tracing/span.go line 499 at r1 (raw file):

it'll do something like this?

yeah, that's what I'm thinking. Except I think the RouteToNotifyListeners should live on s, not on s.Parent.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, and @dt)

pkg/util/tracing/crdbspan.go line 566 at r1 (raw file):

Done.

pkg/util/tracing/span.go line 499 at r1 (raw file):

Done.

pkg/util/tracing/span.go line 480 at r3 (raw file):

Done.

pkg/util/tracing/span.go line 492 at r3 (raw file):

Added a WithListeners option, and removed the register API.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, and @dt)

pkg/util/tracing/span_options.go line 115 at r7 (raw file):

	var notifyParentOnStructuredEvent bool
	parent := opts.Parent.i.crdb
	if parent == nil {

I don't think this protection is necessary; we don't seem to have it in other places. i.crdb is not nilable now, and I think it cannot be made nil-able in the future without a bunch of refactoring.

pkg/util/tracing/span_options.go line 118 at r7 (raw file):

		return false
	}
	parent.withLock(func() {

let's make this a method on parent - maybe WantEventNotifications().

pkg/util/tracing/span_options.go line 475 at r7 (raw file):

// WithEventListeners registers eventListeners to the span. The listeners are
// notified of Structured events recorded by the span and its children. The

s/The listeners are removed when the span is Finish()'ed./Once the span is finished, the listeners are not notified of events any more even from surviving child spans.
(since there's no "removing" to speak of)

pkg/util/tracing/span_options.go line 479 at r7 (raw file):

//
// The listeners will also be notified of StructuredEvents recorded on remote
// spans, when the Recording is imported using sp.ImportRemoteSpans.

s/when the Recording is imported using sp.ImportRemoteSpans/when the remote recording is imported by the span or one of its children.

pkg/util/tracing/span_options.go line 470 at r8 (raw file):

I don't think we need a copy here; I doubt that any caller would benefit from the option to further mutate the slice (and the callee treats it as read-only, so the caller can also keep using it as read-only, right?). If you want to be pedantic, you can specify on WithEventListeners that the caller shouldn't mutate the slice after the call.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @andreimatei, and @dt)

pkg/util/tracing/span_options.go line 115 at r7 (raw file):

Done.

pkg/util/tracing/span_options.go line 118 at r7 (raw file):

Done.

pkg/util/tracing/span_options.go line 475 at r7 (raw file):

Done.

pkg/util/tracing/span_options.go line 479 at r7 (raw file):

Done.

Almost there. Please squash the 2nd commit into the first; I don't think there's a reason to make it separate. And please remind me if there's any benchmark differences to note.
But I'd consider not merging this without a user - at least an imminent user in a perhaps different PR. Cause maybe more things will come to light when we try to use the thing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)

pkg/util/tracing/crdbspan.go line 481 at r9 (raw file):

	for _, span := range children {
		for _, record := range span.StructuredRecords {
			s.notifyEventListeners(record.Payload)

I think it'd be interesting to note on the listener docs that remote events are imported out of order. That may seem obvious for remote events vs local events, but it's not obvious for remote events ingested at the same time.

pkg/util/tracing/crdbspan.go line 558 at r9 (raw file):

	s.mu.Lock()
	defer s.mu.Unlock()
	if !s.mu.parent.empty() && s.mu.recording.notifyParentOnStructuredEvent {

I think notifyParentOnStructuredEvent should be unset when the parent is reset. That'd be cleaner, and allow us here to not test parent.empty().

pkg/util/tracing/crdbspan.go line 560 at r9 (raw file):

	if !s.mu.parent.empty() && s.mu.recording.notifyParentOnStructuredEvent {
		parent := s.mu.parent.Span.i.crdb
		if parent != nil {

get rid of this protection

pkg/util/tracing/crdbspan.go line 596 at r9 (raw file):
Hmm have we talked about the interactions between recording and listeners? The doc currently says

The listeners are notified of Structured events recorded by the span and its children
If things stay as they are, let's make it a lot more explicit that listeners without recording don't do anything.

I'm not sure what the answer should be. For local events (i.e. on the listener's node), there's no cost to passing the events to the listener even if the span is not recording. But that's not true for remote events, and those wouldn't be ingested with the code as written - which is potentially confusing. On the other hand, making WithListeners imply WithRecording(RecordingStructured) adds some cost. We could imagine extensions to the API that make WithListeners imply recording only on the remote children. Or extensions that serialize and "push" the listener aggregators to remote nodes. But it all seems complicated.
If the first user is jobs, who records any way, I'd say let's make WithListeners imply a minimum level of recording (i.e. structured) and document it as such. Of course, if WithRecording(RecordingVerbose) is specified, that should override.

Relatedly, there's the issue of the relationship with the WithForceRealSpan option. If the Tracer is not configured with WithTracingMode(TracingModeActiveRegistry), then specifying WithListeners() in isolation is useless. So, I think WithListeners should imply WithForceRealSpans. That would happen implicitly if we make it imply WithRecording(...).

pkg/util/tracing/tracer.go line 1190 at r9 (raw file):

		traceID, spanID, opName, uint64(goid.Get()),
		startTime, opts.LogTags, opts.EventListeners, opts.SpanKind,
		otelSpan, netTr, opts.Sterile, notifyParentOnStructuredEvent)

Instead of passing notifyParentOnStructuredEvent to newSpan, I think we should leave newSpan alone (it doesn't seem to know about parents at the moment) and do the work below, where we link the new span to the parent (i.e. just below the s.i.crdb.mu.parent = opts.Parent.move() line).
Also, I don't think the opts struct should have the notifyParentOnStructuredEvent(); let's also move that logic here and keep opts dumb. I'm sorry I didn't say that before; I hadn't realized where the code I was reading was.

pkg/util/tracing/span_test.go line 716 at r9 (raw file):

func TestEventListener(t *testing.T) {
	ctx := context.Background()
	tr := NewTracerWithOpt(ctx, WithTracingMode(TracingModeActiveSpansRegistry))

do we need the WithTracingMode option? I don't think we do, since the test is always recording.

pkg/util/tracing/span_test.go line 758 at r9 (raw file):

	sp.Finish()
}

I'd add a test with a parent finishing before a child, and checking that child events are not seen by a listener registered on the parent.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @dt)

pkg/util/tracing/crdbspan.go line 481 at r9 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 558 at r9 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 560 at r9 (raw file):

Done.

pkg/util/tracing/crdbspan.go line 596 at r9 (raw file):

Good point, I've changed the WithEventListeners option to imply RecordingStructured. If a span has a WithRecording(...) we will respect that instead cause it can only be used to set the recording to structured or verbose.

pkg/util/tracing/span_options.go line 108 at r1 (raw file):

Stale comment post refactors, done.

pkg/util/tracing/tracer.go line 1190 at r9 (raw file):

Done.

pkg/util/tracing/span_test.go line 716 at r9 (raw file):

Done.

pkg/util/tracing/span_test.go line 758 at r9 (raw file):

Done.