opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
508 stars 56 forks source link

Flush in tracer #30

Closed jcchavezs closed 7 years ago

jcchavezs commented 7 years ago

Due to PHP language model, we need to flush (to either the backend or somewhere else) on every request (for example with register_shutdown_function) so, since this model is part of the interface, should we encourage implementors to register the flush on construct?

class Tracer extends OTTracer
{
    public function __construct()
    {
        ...
        register_shutdown_function([$this, 'flush']);
    }
    ...
}

That would be convenient as the tracer will flush by default when the http request is finished.

Ping @jonahgeorge @felixfbecker @beberlei @tedsuo @yurishkuro

felixfbecker commented 7 years ago

Let's please not talk in terms of "requests", it can mean a lot of things. Not every PHP process runs as a SAPI module, you can write RPC or HTTP servers in pure PHP. These do not need to flush on every RPC/HTTP "request". So the way I would look at this is how do other languages do this? Does opentracing-javascript register a beforeExit listener to flush?

jcchavezs commented 7 years ago

That is a fair point @felixfbecker. In this case I was talking about the SAPI module but I see the point on the non SAPI ones. Regarding looking at other tracers, OT is a set of interfaces so we might not see this on any OT library but in vendors, right?

In any case, in non request-scoped PHP one flushes on demand and the automatic flush might be strange.

yurishkuro commented 7 years ago

When a tracer is initialized, the constructor typically returns a concrete implementation / class, which can have a close() function, so there's usually no need to expose such function in the OpenTracing API itself. However, in Java this could run into a problem when using the resolver mechanism that instantiates the tracer from the classpath automatically.

jonahgeorge commented 7 years ago

We could add a call to close() in the __destruct() of a Tracer implementation, but in the context of SAPI I would imagine that most people would favor controlling it themselves to ensure they flush after closing the client connection.

jcchavezs commented 7 years ago

Tracer includes a method flush already and I think we all agree should leave implementors to use it in the way it makes more sense for them so I will close this issue.