Trace ID to link before and after events

[fonsp]

I think it is useful to link before and after events. Here is one implementation, using a field trace_id added to events. This also needs to be done to the events defined in RxInfer.jl.

It works, but maybe we can do better?

Alternative 1: before field in AfterSomething events

We can add a field .before that stores the counterpart of an AfterSomething event. Instead of trace_id. (Then the 'before' objectID functions the trace ID.)

Aternative 2: Before{...} and After{...} types

We could change the types to eg:

Before{ProductOfMessagesEvent{V, C, Ms, R}} <: After{Event{:product_of_messages}}

The After type can have a .result field.

Then we can replace this:

# Before
trace_id = uuid4()
invoke_callback(
    context.callbacks,
    BeforeProductOfMessagesEvent(variable, context, messages, trace_id),
)

result = as_message(...)

invoke_callback(
    context.callbacks,
    AfterProductOfMessagesEvent(variable, context, messages, trace_id),
)

with:

# After
result = with_span(
	BeforeProductOfMessagesEvent(variable, context, messages, trace_id)
) do
	as_message(...)
end

[github-actions]

🤖 Code Formatting

Your PR still has some code formatting issues. I've updated PR #590 with the necessary formatting changes.

You can merge that PR into this branch to fix the code style check.

Alternatively, you can run make format locally and push the changes yourself.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (release-6@a40573e). Learn more about missing BASE report.

Additional details and impacted files

@@             Coverage Diff              @@
##             release-6     #589   +/-   ##
============================================
  Coverage             ?   80.74%           
============================================
  Files                ?      212           
  Lines                ?     6462           
  Branches             ?        0           
============================================
  Hits                 ?     5218           
  Misses               ?     1244           
  Partials             ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bvdmitri]

I think storing the .before field might be interesting indeed, the invoke_callback returns the event, so the code should be a simple as

before_event = invoke_callback(callbacks, BeforeEvent(...))

...

after_event = invoke_callback(callbacks, AfterEvent(..., before = before_event)))

[bvdmitri]

I would strongly not suggest doing the with_span idea. I experimented with that a bit, the problem is with Julia's lambda implementation and that it needs to capture variables outside. That hits performance really bad when those variables are not fully inferred (and they are not fully inferred in our case, because the result of the product/rule call/form constrain application is often type unstable).

Also we discussed that a bit in the corresponding RxInferBoard discussion and reached the conclusion that the tracing system should be dead-simple linear and should not have a notion of a span by itself. Spans are OpenTelemetry related and do not exist in TensorBoard for example. So ReactiveMP should not inherently create those spans but it should make it easy to recover those spans (e.g. with extra fields like .trace_id or .before)

[bvdmitri]

The current solution is not bad either btw, so I don't have a strong preference of this implementation over the .before field

[bvdmitri]

I only want to make sure that this extra field does not have performance implications in a sense that if there are no callbacks, Julia compile should be able to remove both callsites. Can you write a test for that to make sure that is what is happening (with the @allocated macro)

[github-actions]

🤖 Code Formatting

Your PR still has some code formatting issues. I've updated PR #590 with the necessary formatting changes.

You can merge that PR into this branch to fix the code style check.

Alternatively, you can run make format locally and push the changes yourself.

[wouterwln]

Super nice, I like this a lot

[bvdmitri]

It seems that Julia v1.10 isn't that smart :( I'm ok with just guarding the test with @static VERSION check and only check on v1.11+

[bvdmitri]

OR - it might also happening because sqrt can throw, an maybe Julia v1.10 isn't good enough to understand that and shows allocations for potential error path. Its better to call functions that truly do not allocate, e.g. +

[github-actions]

🤖 Code Formatting

Your PR still has some code formatting issues. I've updated PR #590 with the necessary formatting changes.

You can merge that PR into this branch to fix the code style check.

Alternatively, you can run make format locally and push the changes yourself.

[github-actions]

🤖 Code Formatting

Your PR still has some code formatting issues. I've updated PR #590 with the necessary formatting changes.

You can merge that PR into this branch to fix the code style check.

Alternatively, you can run make format locally and push the changes yourself.

[bvdmitri]

lets gooo!

[fonsp]

I'll work through the CI Julia 1.10 and then I merge 👍

[fonsp]

For future reference, here is a function that matches traces based on their ID:

function find_spans(trace_events::Vector{TracedEvent})
	# Found spans
	spans = Tuple{TracedEvent,TracedEvent}[]
	# Currently waiting to be matched
	lonely = OrderedDict{Base.UUID, Any}()
	# No match possible
	not_matched = TracedEvent[]

	for te in trace_events
		event = te.event
		if hasfield(typeof(event), :trace_id)
			tid = event.trace_id
			friend = get(lonely, tid, nothing)
			if !isnothing(friend)
				delete!(lonely, tid)
				push!(spans, (friend, te))
			else
				lonely[tid] = te
			end
		else
			push!(not_matched, te)
		end
	end

	push!(not_matched, values(lonely)...)

	return (; spans, not_matched )
end