Fix decoration of symfony scoped http clients

[simonberger]

This MR Fixes #700 and resolves #701

I had a look how this bundle decorates the http-client. It iterates over all tagged http client services and creates new decorated services of them. This is not the intended decoration way and (see update 1) should also not be necessary (?).

Instead just the default http_client needs to be decorated which is put into all other (scoped) services.
This changes the Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient not to be the first service in the decoration chain anymore, but this is hopefully not important.

Note:
I did not test the function of sentries TraceableHttpClient before or after my change, I just tested the decoration chain to be intact for the default http client or when used as a scoped service.
I tested it in Symfony 6.2 and can test this next week also in Symfony 5.4 which has no bigger internal changed behavior in this regard tho.

Update 1:
Removed the comment ragarding suggested way to decorate all different http client services because there actually ia none.

[ste93cry]

This is not the intended decoration way and should also not be necessary

Actually, it does it this way because Symfony does the exact same thing here. Either we're missing something, or it seems strange

[simonberger]

Yeah, I know this had to be the source. First of all I am by far no decoration expert and just learned a lot of the container building in the last weeks.
I indeed cannot find something official the way we are doing it but to me it is simple, straight forward and I cannot see a "reasonable" scenario it fails. One could be self created services tagged by http_client.client which is no promoted tag in any way.
Symfony in the few extension descriptions it has, just points to a general decoration document which surely does not help here.

[ste93cry]

Would you mind checking if you see the same behavior you’re reporting for the TraceableHttpClient used by Symfony to collect the data for the profiler?

[simonberger]

@ste93cry do you mean if the TraceableHttpClient is actively used / part of the decoration? As I wrote at the end of #700 it does not happen with TraceableHttpClient being part of the chain.
It happened on our servers and took me quiet some time to find the root cause in my environment where debugging and the webprofiler are active.

[simonberger]

Another general problem you'll face when decorating every tagged client and being placed in front of the ScopingHttpClient is to not having the complete URL.
Depending on the type of the scoped client you'll often just receive the relative url and the base_uri is prefixed by the ScopingHttpClient which all later clients receive in the request.
I noticed that the URL is also handled by your client which does not work correctly then.

[simonberger]

I am surprised this and more importantly the underlying bug is still open. IMO the current state of the http client
tracing implementation is error prone and can lead to various problems (as for me) which stay maybe undetected. Especially as it is activated by default.

Talking of this specific PR Iam relatively confident it does its job well but it needs some tests/confirmation.

I do not wait for it tho, because we decided to disable the tracing. At least for now.

[ste93cry]

I tested the changes and I'm still having issues. Symfony 6.3+ reworked the way the HTTP client the decoration is done, so it should be pretty easy to fix the issue we're having here by simply decorating the http_client.transport service. However, for older versions of the framework, I still have to find a way that works: the problem is that they're using some weird way to decorate the services based on referencing the *.inner service directly instead of using the classic decoration approach supported by the framework, and which service is referenced as raw HTTP client is hardcoded🤦

[simonberger]

Can you tell the kind of problems?

[simonberger]

Wasted enough energy trying to convince it is a problem necessary to fix and this likely the correct solution.

[ste93cry]

I'm sorry that you feel like you've wasted energy, the reported issue is clearly a bug and noone doubts that needs to be addressed. Unfortunately, this package is managed by a couple of volunteers and we often don't have all the time and resources to respond as quickly as expected. Regarding the issues that I have with your solution, I've observed that the decoration is not applied in all cases, depending on the configuration you have. Assuming a config like the one below, the http_client service is decorated, while the scoped clients are not.

http_client:
  default_options:
    retry_failed: true
  scoped_clients:
    noretry.client:
      retry_failed: false
      base_uri: https://example.com/
    example.client:
      retry_failed: true
      base_uri: https://example.com/

With older versions of the frameworks, I've observed even worse scenarios, e.g. other decorators disappearing from the chain. This is not to say that the proposed solution is the wrong way, but it needs a lot more work to be reliable, apparently.

[simonberger]

@ste93cry this is indeed an issue that can be fixed by increasing the decoration priority to let's say 15. It needs to exceed the RetryableHttpClient which has priority 10.
I did not notice because we do not use this feature

[ste93cry]

Unfortunately, I tried to change the priority and it still didn't fix the issue, in particular on versions of the framework prior to 6.3. I suggest setting up a small project running those versions and trying for yourself these changes to see what I'm talking about

[simonberger]

I tested the fix just an hour ago on Symfony 4 and 5 with and without retryable and on 5.4 with multiple decorations.
Did you look for the decoration in the whole chain? It comes after the scoped client.

[ste93cry]

I tried again, and I confirm what I already mentioned:

Symfony 6.3

• Setting the http_client.mock_response_factory config option makes the decoration fail for the scoped clients: I tried setting both a negative and positive decoration priority, but it didn't matter.
• Enabling the UriTemplateHttpClient by installing one of the supported and required packages, e.g. guzzlehttp/uri-template makes the decoration fail: it works for the default HTTP client, but not for the scoped clients, no matter the decoration priority.

Symfony 5.4

• Setting the http_client.mock_response_factory config option makes the decoration fail: I tried setting both a negative and positive decoration priority, but it didn't matter.

[simonberger]

@ste93cry I cannot follow all points, but I now first tested it in Symfony 6.3. The http_client.transport introduction brought a breaking change no longer have a working decoration when scoped clients come into play.

I didn't want to try to find a way around it directly or file a Symfony bug.

Instead I pushed a new commit to my branch which fixes the decoration for Symfony 6.3 by using the http_client.transport service from this version on. The FrameworkBundle 6.3 feature detection was pretty tricky as there was almost no new feature and no single new class out of test scope which I did not want to use.
I tested this change in Symfony 5.4, 6.2 and 6.3 without debug activated (only) and with active retryable.
I still don't know why you do not get it to work in versions prior to 6.3 if that should be the case.

Regarding mock_response_factory I did not test this. It probably is working in Symfony 6.3 now because of the transport service introduction. Not sure about earlier versions. I see no value in expecting the decoration there or is the http-client feature completely broken?

[ste93cry]

Instead I pushed a new commit to my branch which fixes the decoration for Symfony 6.3 by using the http_client.transport service from this version on

This is indeed a part of the fix that we need. Since 6.3, it looks like they finally got the decoration right, so that's one less problem we have to tackle.

The FrameworkBundle 6.3 feature detection was pretty tricky

It should be enough to just check that the http_client.transport service exists in the container. If it is, then we have to decorate it, otherwise, we have to fallback to another mechanism to get things right.

I tested this change in Symfony 5.4, 6.2 and 6.3 without debug activated (only)

I tested it using the dev environment, which adds yet another decorator to the chain. When using the http_client.mock_response_factory, there's no apparent way to make the decoration to work by using the standard decoration approach.

I see no value in expecting the decoration there or is the http-client feature completely broken?

We should get things right for all cases, otherwise, depending on which option you configured, you would see different behaviours which are hard to track and understand. I'm working on an improved solution, it seems to work but I need to polish it and find a meaningful way to test it.

[simonberger]

It should be enough to just check that the http_client.transport service exists in the container.

You can try but as far as I saw the definitions are not there at this point and even they would I don't know how safe it is to rely on it.

We should get things right for all cases, otherwise, depending on which option you configured, you would see different behaviours which are hard to track and understand

The mock_response_factory is for testing purposes of the own project using the http-client to simulate some specific responses. I see no realistic scenario they would care a sentry decoration is or isn't there.
Especially when it should work in never versions from this point on.

[ste93cry]

Since you pushed new commits after closing this PR, I cannot reopen it, hence I've submitted my solution in a new one. I tested it on all versions of the framework we support, both in development and production mode, with both the retryable and the mock client enabled/disabled. It seems to work fine, but if you could make some more tests on your own it would be great!