[opentelemetry-php-contrib] Symfony HttpClient - Add the possibility to ignore middlewares for instrumentation

[jvocampings]

Hi everyone,

I'd like a modification to the instrumentation of Symfony HttpClient. Actually it is implemented here: https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Symfony/src/HttpClientInstrumentation.php

Is your feature request related to a problem? When I monitore HTTP request with OpenTelemetry, logs are polluted by calls to each HttpClient middlewares. In my case, I don't need to see each call to the request method of TraceableHttpClient, UriTemplateHttpClient, ScoppingHttpClient etc... They are irrelevant in my case.

In fact I just need to see the call to CurlHttpClient because it executes the HTTP request I need to monitore.

Describe the solution you'd like I have ideas to update OpenTelemetry\Contrib\Instrumentation\Symfony\HttpClientInstrumentation. Tell me which one you'd like. Or maybe another I don't know.

With environment variable Add new env variable to set the class name for the hook. Default value is Symfony\Contracts\HttpClient\HttpClientInterface

Or add new env variable to exclude (like synchronous clients) a list of class names. Default value is empty.

Then update the method in consequences.

Add argument to the method register

The class name for the hook.

public static function register(string $className = HttpClientInterface::class): void

or a list of class names to ignore.

public static function register(array $ignoreClassNames = []): void

That means we have to disable instrumentation with OTEL_PHP_DISABLED_INSTRUMENTATIONS and call ourselves HttpClientInstrumentation::register() with the new argument. Then maybe we should also add an env variable to disable the autoload for HttpClientInstrumentation.

[brettmc]

I think the spec authors intended it to be done via with TracerConfig - it's been part of our SDK for a while, but not implemented in any instrumentation modules. Conceptually, I think that each different hooked method in a module would have a different name, eg io.opentelemetry.contrib.php.symfony_http.client, io.opentelemetry.contrib.php.symfony_http.middleware etc, which allows configuration to disable parts of instrumentation by pattern matching like in the linked test above.

edit: and would be configured through declarative config, example: https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L765

[jvocampings]

Thank you for your answer.

Not sure to understand. You prefer having a trcer for each client and each middleware ?
e.g. io.opentelemetry.contrib.php.symfony_http.curl_http_client, io.opentelemetry.contrib.php.symfony_http.retryable_http_client, io.opentelemetry.contrib.php.symfony_http.scopping_http_client etc ?

[brettmc]

Not sure to understand. You prefer having a trcer for each client and each middleware ?

I'm only speaking generally, because I don't know symfony deeply and didn't write the instrumentation. Using TracerConfig can be a future goal, given that it's not widely used yet. I think there are 3 broad approaches:

1. an env var (as you've proposed)
2. using SPI and declarative config (the spec for this is still in-development, so it's not ready, but it's prototyped in https://github.com/open-telemetry/opentelemetry-php/blob/main/examples/src/ExampleInstrumentation.php)
3. TracerConfig

Of your two approaches (opt-in or opt-out), which do you think is more ergonomic? Ideally, the default behaviour would match the current functionality. Another problem you will run in to is that there is a disconnect between pre and post hooks (that's how the underlying observer API works) - so to avoid having post hooks ending the wrong span if pre hook decided not to start one, you might need to have the pre hook create and activate a non-recording span. Untested, but perhaps:

if (not_tracing) $span = Span::getInvalid();
Context::storage()->attach($span->storeInContext(Context::getCurrent()));

[jvocampings]

Sorry for the delay.

Of course the default behavior must match the current functionality to avoid any bc break.

I think it is better to wait for the SPI and declarative config. It seems more flexible to me.

In the meantime, I can just fork the HttpClientInstrumentation or create my own hook. It is not a big problem in my case to fork it.

[smaddock]

I think the spec authors intended it to be done via with TracerConfig - it's been part of our SDK for a while, but not implemented in any instrumentation modules. Conceptually, I think that each different hooked method in a module would have a different name…

Not to derail this issue from Symfony, but to implement TracerConfig in an instrumentation library, the provider would just need separate CachedInstrumentation per hook, correct?

[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.

[stale[bot]]

This issue has been automatically closed because it has not had recent activity, but it can be reopened. Thank you for your contributions.