Skip to content

[HttpClient] Add note about a required decoration priority > 5 #18087

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

Closed

Conversation

simonberger
Copy link

In symfony/symfony#47836 the decoration priority of the TraceableHttpClient services had been increased to 5. This results in the own decorator with a default priority of 5 is not added to the chain of any scoped client.

As this is not obvious and caused some trouble for us, I like to add the info to the documentation.

Increasing the priority is only needed when TraceableHttpClient is decorated but I am not sure how to word the condition, nor if this is really important to tell here.

@simonberger simonberger force-pushed the http-client-decoration-priority branch from 756b175 to 5f875cf Compare March 19, 2023 13:42
@simonberger
Copy link
Author

simonberger commented Mar 19, 2023

This PR assumed a decoration like this is the intended way:

services:
     decorated.http_client:
          class: HttpClientDecorator
          arguments: [decorated.http_client.inner]
          decorates: http_client
          decoration_priority: 10

Issues like this getsentry/sentry-symfony#700 show, it is not clear how to decorate (all ways http clients can be bootstrapped) in the correct way.

@OskarStark
Copy link
Contributor

Friendly ping @nicolas-grekas

@nicolas-grekas
Copy link
Member

I'm not sure it makes sense documenting this. What Sentry does is uncommon to me.

@simonberger
Copy link
Author

I searches around how the http client had been decorated in examples, issues and more. It shows there is a bigger diversity but in general there aren't that many sources, if you search for help.
Decoration is communicated to be the key extension tool and imho it is not enough to just point to the general decoration document while it is unclear what exactly to decorate and how it behaves in the different ways the clients can be bootstrapped.
It is kinda offtopic to the current state of the PR, but I could extend it to make decoration more clear and show the intended way.

Here are sources where others decorated the scoped clients:

If the tagged clients are decorated, the decorator does not receive the final URL which should cause issues in may use cases.
Additionally the decorator is twice in the chain as the sentry issue shows. I did not really debug how this happen, but most likely because the default http_client service as well as the scoped http service are decorated and "merged together" in a later step.
On the other hand If the http_client service is decorated, we need a minimum priority of 5/6 because otherwise the scoped clients are already decorated, when our decoration would be processed which could maybe considered an resolvable issue. At least to me it is hidden knowledge you can just find out by try and error or could end up doing exactly this tagged client decoration, because your own decorator does not appear otherwise.

@nicolas-grekas
Copy link
Member

This made me realize that symfony/symfony#49513 should have solved this issue with decoration. This could be worth documenting.
(But I wouldn't document what you describe, it's too niche to me and it should be obsoleted by this PR.)

@simonberger
Copy link
Author

simonberger commented Mar 21, 2023

I prefer a different way of documenting this as well, especially would prefer a fix for this :)
To me this is no niche scenario tho. When sentry fixes its decoration, they are most likely very much effected to decide of a priority and cant stick to zero.
Actually everyone is affected that want to use scoped clients or third party libraries like sentry that do not know how the clients are created.

@OskarStark
Copy link
Contributor

@javiereguiluz
Copy link
Member

After reading the entire discussion, let's close this one as "won't merge" ... and let's add the missing docs about the new HttpClient decoration strategy in #19363. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants