Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use NoopTracerProvider if Sdk is disabled #55

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

GCalmels
Copy link
Collaborator

OpenTelemetry already use Sdk::isDisabled() to use NoopTracerProvider if env OTEL_SDK_DISABLED is set to true.
Like this, we can add this env on phpunit.xml file

@GCalmels GCalmels self-assigned this Jun 11, 2024
@GCalmels GCalmels added the enhancement New feature or request label Jun 11, 2024
@GCalmels GCalmels requested a review from cdaguerre June 11, 2024 10:19
readme.md Outdated Show resolved Hide resolved
readme.md Outdated
- Using the official [OpenTelemetry SDK](https://github.com/open-telemetry/opentelemetry-php)
- Minimal auto-instrumentation for **requests**, console **commands**, **consumers** and **doctrine**
- Trace context propagation from incoming **requests** to **consumers**, **outgoing http calls** and **databases** (using [`sqlcommenter`](https://google.github.io/sqlcommenter/))
- Configurable blacklisting of requests by path to avoid useless traces, eg. `/metrics` or `/_healthz`
- Automatic log inclusion with configurable log level and channels
- Disabling tracing with the `NoopTracerProvider` thanks to OTEL_SDK_DISABLED=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think it's worth mentioning here

@@ -46,6 +48,9 @@ public static function setProvider(TracerProviderInterface $tracerProvider): voi

private static function getProvider(): TracerProviderInterface
{
if (Sdk::isDisabled()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not the right place to handle it because it will not handle a disabled SDK within Symfony, only for static usage.

Don't you think we should rather adapt the TracerProvider in Symfony itself?
Something by the lines of:

<?php

declare(strict_types=1);

/*
 * This file is part of the worldia/instrumentation-bundle package.
 * (c) Worldia <[email protected]>
 */

namespace Instrumentation\Tracing;

use OpenTelemetry\API\Trace\NoopTracer;
use OpenTelemetry\API\Trace\TracerInterface;
use OpenTelemetry\API\Trace\TracerProviderInterface;
use OpenTelemetry\SDK\Sdk;

class TogglableTracerProvider implements TracerProviderInterface
{
    public function __construct(private TracerProviderInterface $decorated)
    {
    }

    public function getTracer(string $name, string|null $version = null, string|null $schemaUrl = null, iterable $attributes = []): TracerInterface
    {
        if (Sdk::isDisabled()) {
            return new NoopTracer();
        }

        return $this->decorated->getTracer($name, $version, $schemaUrl, $attributes);
    }
}

With :

use Instrumentation\Tracing\TogglableTracerProvider;
use OpenTelemetry\API\Trace\TracerProviderInterface;

return static function (ContainerConfigurator $container) {
    $container->services()
        ->set(TogglableTracerProvider::class)
        ->decorate(TracerProviderInterface::class)
        ->args([
            service('.inner'),
        ])
    ;
};

WDYT?

@GCalmels GCalmels requested a review from cdaguerre August 6, 2024 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants