fix(tracing): avoid assigning span's local root to self, so that the python GC doesn't kick in

[danielshaar]

We (Modal Labs) noticed that enabling tracing in parts of our code caused the Python GC to kick in a lot and add large delays to our event loop. A simple repro (see test) showed us that there was a self reference cycle in span when a top-level span's local root was assigned to the span itself, preventing Python from cleaning up the objects by reference counting.

The change itself is pretty straightforward - we use None to represent when a span's local root is itself. The span still exposes _local_root with the same behavior via a property, and internally stores a _local_root_value that holds a non-self-referential span. This also means that the tracer no longer needs to explicitly set the local root for top-level spans. This should be fairly non-risky as it doesn't change any code behavior.

Checklist

• PR author has checked that all the criteria below are met
• The PR description includes an overview of the change
• The PR description articulates the motivation for the change
• The change includes tests OR the PR description describes a testing strategy
• The PR description notes risks associated with the change, if any
• Newly-added code is easy to change
• The change follows the library release note guidelines
• The change includes or references documentation updates if necessary
• Backport labels are set (if applicable)

Reviewer Checklist

• Reviewer has checked that all the criteria below are met
• Title is accurate
• All changes are related to the pull request's stated goal
• Avoids breaking API changes
• Testing strategy adequately addresses listed risks
• Newly-added code is easy to change
• Release note makes sense to a user of the library
• If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
• Backport labels are set in a manner that is consistent with the release branch maintenance policy

[platers]

We at Suno have also found memory leaks when tracing is enabled, causing us to disable them in production. This seems likely to be the cause, would love to see this merged!

[quinna-h]

@danielshaar Thanks for the contribution and for highlighting this issue! Someone from our team will be assigned to this shortly.

[erikayasuda]

Hi @danielshaar , thanks for flagging this and creating a fix PR. Unfortunately our CI currently doesn't let external contributors trigger the GitLab portion of our test suite (we are actively working on a fix for this). To get around this for now, I went ahead and duplicated your PR here: #10809. This should run the entirety of our test suite.

I just took a quick skim, and I do think this will fail some of our tests due to other parts of the repo referencing _local_root. I'll take a look at what needs to be adjusted, and get back to you with an update on that PR.

Closing this PR in favor of the duplicate.

[danielshaar]

Anything that references _local_root should go through the property accessor I created that preserves the previous behavior, so this breaking tests would surprise me.

[erikayasuda]

Anything that references _local_root should go through the property accessor I created that preserves the previous behavior, so this breaking tests would surprise me.

My bad I missed that. Yes I think we should be good then, the test suites seem to be passing on the other PR.