fix(tracing): do not raise exception if partial flush is triggered without any spans

[romainkomorndatadog]

Adds a guard against on_span_finish() with partial flushing on running into an IndexError because there are no spans to flush (which may happen if tracer.configure() was called between the time a span was created and the time it was finished).

In practice, this turns into:

>>> import ddtrace
>>> with ddtrace.tracer.trace("regression"):
...     ddtrace.tracer.configure(partial_flush_min_spans=1)
...
Partial flush triggered but no spans to flush (was tracer reconfigured?)

This also refactors the test for our os.fork() wrapper to have the child process unpatch coverage (just in case, since it occasionally causes exceptions on exit) and exit cleanly (otherwise it would continue running other tests which is not what we want).

Checklist

• Change(s) are motivated and described in the PR description
• Testing strategy is described if automated tests are not included in the PR
• Risks are described (performance impact, potential for breakage, maintainability)
• Change is maintainable (easy to change, telemetry, documentation)
• Library release note guidelines are followed or label changelog/no-changelog is set
• Documentation is included (in-code, generated user docs, public corp docs)
• Backport labels are set (if applicable)
• If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

• Title is accurate
• All changes are related to the pull request's stated goal
• Description motivates each change
• Avoids breaking API changes
• Testing strategy adequately addresses listed risks
• Change is maintainable (easy to change, telemetry, documentation)
• Release note makes sense to a user of the library
• 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

[datadog-dd-trace-py-rkomorn]

Datadog Report

Branch report: romain.komorn/AIT-10242/dont_crash_when_spans_list_empty
Commit report: 13f14b6
Test service: dd-trace-py

❌ 1 Failed (0 Known Flaky), 169460 Passed, 1077 Skipped, 9h 44m 55.2s Total duration (25m 13.56s time saved)

❌ Failed Tests (1)

• test_slow_imports - test_serverless.py - Details

  Expand for error

   Expected status 0, got 1.
   === Captured STDOUT ===
   === End of captured STDOUT ===
   === Captured STDERR ===
   Traceback (most recent call last):
     File "/root/project/ddtrace/internal/packages.py", line 250, in is_distribution_available
       import importlib.metadata as importlib_metadata
     File "tests/internal/test_serverless.py", line 125, in find_spec
       raise ImportError(f"module {fullname} was imported!")
   ImportError: module importlib.metadata was imported!
   ...
  

[brettlangdon]

can we add a regression test?

the simple case we had should work well:

with tracer.trace("test"):
    tracer.configure(partial_flush_enabled=True, partial_flush_min_spans=1)

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-06-18 08:06:16

Comparing candidate commit a49062d in PR branch romain.komorn/AIT-10242/dont_crash_when_spans_list_empty with baseline commit 0fb7afa in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 221 metrics, 9 unstable metrics.

[brettlangdon]

do we want to backport this? I would think no since it isn't a bug fix. if there isn't a release note we cannot release a patch with this anyways, so it would have to wait for another change to be backported.

we might want to change this to a fix(tracing): do not raise exception if partial flush is triggered without any spans, add a release note, and then keep the backports. wdyt?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 17.64706% with 28 lines in your changes missing coverage. Please review.

Project coverage is 10.10%. Comparing base (deadfcd) to head (1193055).
Report is 9 commits behind head on main.

Files	Patch %	Lines
ddtrace/_trace/processor/__init__.py	33.33%	12 Missing ⚠️
tests/tracer/test_processors.py	0.00%	12 Missing ⚠️
tests/contrib/subprocess/test_subprocess.py	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #9349       +/-   ##
===========================================
- Coverage   75.61%   10.10%   -65.51%     
===========================================
  Files        1336     1347       +11     
  Lines      125991   125077      -914     
===========================================
- Hits        95271    12643    -82628     
- Misses      30720   112434    +81714     

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

[romainkomorndatadog]

So after yet another twist and turn... @brettlangdon and I decided that we don't want to alter the behavior of the span aggregator in this PR.

Any spans opened prior to forking could end up getting submitted if they're closed in the forked process (which is not currently the case). Deciding whether that's desirable behavior is outside of the scope of this change.

I'm leaving in the change to the test_fork() test just because it bugs me that we're forking and not having the child exit out as soon as it can, though.

[github-actions]

The backport to 2.7 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.7 2.7
# Navigate to the new working tree
cd .worktrees/backport-2.7
# Create a new branch
git switch --create backport-9349-to-2.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fffab017cb2de72d10b35585350c7fa65756e785
# Push it to GitHub
git push --set-upstream origin backport-9349-to-2.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.7

Then, create a pull request where the base branch is 2.7 and the compare/head branch is backport-9349-to-2.7.