RFI: How to close all open spans and scopes

[huggsboson]

Thanks for the wonderfully updated library. We were using a very old version with memory leaks and tons of bugs, it's great to see this getting love and adhereing to the specs.

We have a pattern in our application where it will randomly redirect and exit within certain requests. For those cases with the old lib we had a pattern in the exit handler where we would end() any live spans up the tree so the spans would get sent out but with the new library it's much less clear how to do that. Additionally the new DebugScope will trigger_error all over the place for the left open scopes.

The thing I was thinking about doing is (I haven't tested this, wanted to get feedback first in case I was missing anything obvious):

// Need to handle getting to the root
while ($context = Context::getCurrent()) {
    $span = Span::fromContext($context);
    $span->end();
    // Needs to be against the contextStorage not the context itself....
    $context->detach();
}

Is there a more elegant way to do what I'm trying to do than this for this ugly situation that is out of my control?

[brettmc]

That's pretty much the pattern I would use, and I don't think there is a more elegant way. It doesn't look quite right visually, though. You detach a scope, not a context. This (untested) snippet might be closer to what you want (which is based on how our auto-instrumentation packages do it):

while ($scope = Context::storage()->scope()) {
  $scope->detach();
  $span = Span::fromContext($scope->context());
  $span->end();
}

One thing that you might also need to check for is that if you have started any spans but not activated them, I think they would be lost.

[huggsboson]

Thank you so much for the fast and helpful response! Will try that out / looking at the auto-instrumentation.

[huggsboson]

A few notes, I made a contract test for this to ensure the behavior remains stable:

public function testOtelDanglingSpanContractTest()
{
    // Reset storage
    Context::setStorage(null);
    $this->assertEquals(Context::getCurrent(), Context::getRoot());

    $tracer = NoopTracer::getInstance();
    $span1 = $tracer->spanBuilder("span1")->startSpan();
    $scope1 = $span1->activate();
    $this->assertNotEquals(Context::getCurrent(), Context::getRoot());
    // Intentionally no detach call

    // Will trigger error without this
    // Walk tree through statics
    $ctx = Context::storage();
    while ($ctx->current() !== Context::getRoot()) {
        $span = Span::fromContext($ctx->current());
        $span->end();
        $ds = new \OpenTelemetry\Context\DebugScope($ctx->scope());
        $ds->detach();
    }
}

Given that a bunch of these things are internal it may be better to add this to the SDK for folks that need it.

A few notes:

1. We had context leaking across tests so it would be helpful to add a reset utility somewhere that lets it get back to null again
2. DebugScope is internal, another reason to add this to the core.
3. getRoot is also internal.

[huggsboson]

I made PR #1140 let me know what you think.

[brettmc]

@huggsboson DebugScope can now be disabled entirely, since #1205 so with that turned off you won't get the notices.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.