wodby / php

Generic PHP docker container images
MIT License
155 stars 103 forks source link

Add opencensus PECL package for opentracing instrumentation #80

Closed virtuman closed 4 years ago

virtuman commented 5 years ago

Installing this PECL extension allows to instrument php code with opentracing. Opencensus provides several exporters: Zipkin, Jaeger, and others

This particular version of the extension has support for WordPress integrated but can be used with any other php framework, just a question of setting your own things to trace with opencensus_trace_method(.., ..) function

Instrumentation of the code is something as simple as adding a couple of lines to compose.json, example:

  "require": {
    ......
    "opencensus/opencensus": "^0.5.*",
    "opencensus/opencensus-exporter-jaeger": "~0.1"
  },

and a couple of lines as close to the start of the application as possible will provide wordpress database and cache layer tracing:

\OpenCensus\Trace\Integrations\Wordpress::load();
\OpenCensus\Trace\Integrations\Mysql::load();
\OpenCensus\Trace\Integrations\Curl::load();
\OpenCensus\Trace\Integrations\Memcached::load();

opencensus_trace_method('wpdb', 'query');
\OpenCensus\Trace\Tracer::start(new \OpenCensus\Trace\Exporter\JaegerExporter('your-service-name', ['host' => 'jaeger-host']));
csandanov commented 5 years ago

The extension is in the alpha state with the last released more than a year ago. Also I see the official zipkin php package doesn't use opencensus and you can use jaeger without it as well. There's also https://github.com/opentracing/opentracing-php. So I don't think it's worth adding it to the generic image

virtuman commented 5 years ago

this alpha release allows to instrument wordpress and other frameworks and for all intents and purposes - online community deems it to be stable for as long as it was out. not entirely sure why it's still marked alpha and stable hasn't been released. used in heavy production by many, including ourselved, on very heavy trafficked sites

other implementations of opentracing do not seem to be easily integratabtle with wordpress, as they require closing spans and traces at the end of the call, which with wordpress is a little problematic, short of writing your own error handler and shutdown register functions, which is much easier to screw up by the individual developers and it is just a much longer road to the same result but less error-prone.

Having the PECL in the image - won'e hurt anyone who's not using it, and while opentracing has been around for some time, wordpress community specifically is not exactly the fastest to adopt things, even more-so things that are specialized for tracing and heavy traffic.

This extensions is fairly light-weight, and gives a huge power to all wodby/php users, whether wordpress or anything else. and let' not confuse opentracing and opencensus. latter integrates opentracing, but provides many extras outside of just opentracing, ie. metrics, etc.

much needed module, and we could try to experiment with the non-alpha version, to see if instrumentation of wordpress sites is still possible with non-alpha. I wouldn't mind testing, if you show any interest in adopting this into the wodby/php image.

Thank you for consideration

csandanov commented 5 years ago

There was an announcement recently to merge opencensus with opentracing: https://www.infoq.com/news/2019/04/opencensus-opentracing-merge/ https://medium.com/opentracing/merging-opentracing-and-opencensus-f0fe9c7ca6f0

virtuman commented 5 years ago

announcement looks promising, but is there a reason to wait? we can adopt new merged project when it is released and production ready, i can stay on top of that and create a relevant PR when it's ready, but to allow users to begin adoption of open tracing - it would be great to have this PR merged and provide this open tracing support sooner rather than keep on waiting until it becomes available some unknown time later?

This currently proposed change assumes no risks, and users chose to OPT-IN only if they use functionality, otherwise it's just a loaded module.

Lack of this module has become a blocker for us, therefore i'm here again, hoping that merge of this PR can still happen.

virtuman commented 4 years ago

any chance you could re-visit this? thank you

csandanov commented 4 years ago

so, for now the answer is no, feel free to reopen if they release a stable version on PECL