[BUG] reset() not defined in Tracer interface

[josesa]

Bug description

In certain cases where trace instrumentation needs to be done manually, reset() needs to be called (is defined as public) but the NoopTracer doesn't implement it.

Tracer Interface: https://github.com/DataDog/dd-trace-php/blob/master/src/api/Contracts/Tracer.php

Is this an explicit design decision?

PHP version

Any

Tracer version

0.70.1

[bwoebi]

Hey José,

Tracer::reset() is mostly used by tests for ensuring proper state. It does not actually reset the whole tracer (anymore).

What is the concrete use case you have in mind? Discarding all spans? Discarding some recently closed spans? Stopping instrumenting alltogether?

The current recommended way to cleanup instrumentation would be:

ini_set("datadog.tracer.enabled", 0);

To re-activate, set it to 1 again then:

ini_set("datadog.tracer.enabled", 1);

This will completely shutdown tracing functionality and start it back up from a clean state.

[josesa]

We have a long process and for error handling, instrumentation is created manually with

DD_TRACE_GENERATE_ROOT_SPAN=false

and then

$tracer = DDTrace\GlobalTracer::get(); 
$tracer->startActiveSpan()

On each process we want to track, we need to properly finalise it:

$scope->close();
$tracer->flush();
$tracer->reset();

For development purposes, the NoopTracer is sometimes used and of course the reset() then fails.

[bwoebi]

You can just remove $tracer->reset() there, it is not needed.

In fact, we might remove this reset function any time. It's not on the interface, because its existence should not be relied on.

[allenisalai]

Hi Bob,

In our use case, we are also profiling a long running symfony command that processes large numbers of batches. We have listeners that start the instrumentation process, but we want to discard any initialization spans and track each batch as a root span.

The general flow is like this:

// DD_TRACE_GENERATE_ROOT_SPAN=false

// in listener, outside of the command
GlobalTracer::get()->startRootSpan($operationName, [
            'ignore_active_span' => true,
            'finish_span_on_close' => true,
        ]);


// In command::execute();
// get through the process initialization and to the loop
GlobalTracer::get()->reset();

foreach($batches as $batch) 
{
    $scope = GlobalTracer::get()->startRootSpan($operationName, [
            'ignore_active_span' => true,
            'finish_span_on_close' => true,
    ]);

    // do work

    $scope->close();
}

Is it still recommended to disable/re-enable like this instead of using Tracer::reset(). If so, are there any caveats to be aware of (performance implications, partial spans etc).

ini_set("datadog.tracer.enabled", 0);
ini_set("datadog.tracer.enabled", 1);

To get it to work, I also had to reset the GlobalTracer with GlobalTracer::set(new Tracer()) since the internal Tracer instance wasn't reset. With the proposed implementation to reset the tracer state in https://github.com/DataDog/dd-trace-php/issues/1533#issuecomment-1059211743, the resetting implementation is leaked in user land. If extending the Tracer interface isn't an option, a GlobalTracer::reset() method would help provide an abstraction around this functionality.

[PROFeNoM]

Hi @allenisalai!

The resetting implementation is leaked in user land

What exactly do you mean by this? What exactly is leaked? What are you experiencing that you didn't expect? Do you have a sample example :eyes:?

A GlobalTracer::reset() method would help provide an abstraction around this functionality.

I personally agree that it shouldn't make any harm, and makes relative sense :) Still, I'd like to understand and fix anything that is wrong with the current Tracer::reset() method.

If so, are there any caveats to be aware of (performance implications, partial spans etc).

Well, while I cannot quantify it, shutting down the tracer and starting it back up certainly must have some overhead associated with it. However (maybe I'm missing some code or context), why do you need to reset the tracer here? Isn't starting a new root span enough to track each batch as a root span?

[allenisalai]

Hi @PROFeNoM,

Thanks for your time. :)

Nothing is wrong with Tracer::reset() and it does everything I expect, but my concern was to have my code depend on it after reading Bob's comment

In fact, we might remove this reset function any time. It's not on the interface, because its existence should not be relied on.

I was suggesting that if you don't want to add reset() to the Tracer interface, adding at the GlobalTracer level would be nice to keep the functionality abstracted for users.

why do you need to reset the tracer here? Isn't starting a new root span enough to track each batch as a root span?

I want to discard the currently running active span and not send it to datadog. Resetting the tracer seemed like the only way to do that. Is there a better way to discard a span?

[bwoebi]

Hey @allenisalai,

Yes, a GlobalTracer reset might make sense, I agree.

I want to discard the currently running active span and not send it to datadog.

There are two use cases to be distinguished: a) Do you not want to have metrics about that span at all? And not trace it. b) Do you want to have metrics (how often the span happens), but not send span data to reduce traffic / cost?

The former is done by resetting, the latter can be controlled by setting sampling priority.

[allenisalai]

Hi @bwoebi,

My use case is aligned with scenario a so it seems like I'm doing the right thing. Thank you for the clarification.