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
Tracing: symfony http client #606
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. I did a first round of code review and left some comments here and there. Besides adding a CHANGELOG entry and some more tests, this is a good foundation for sure for this feature and I can't wait to see it merged 🥳
17b42ba
to
a951725
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build is broken. Moreover, there's a lot of untested code (especially in the AbstractTraceableResponse
class). It would be great if you could add at least a test for each method, which tests both the success path and the failure one (if there is any)
tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php
Outdated
Show resolved
Hide resolved
tests/DependencyInjection/Compiler/HttpClientTracingPassTest.php
Outdated
Show resolved
Hide resolved
@ste93cry Why has this been closed? I did not have time to complete the tests in the last weeks, but I will work on this in the few days. |
Sorry, I did not meant to close it. I mistakenly deleted the |
This comment was marked as outdated.
This comment was marked as outdated.
68a2963
to
61b7a5e
Compare
I've added some tests and rebased the branch. All issues should be resolved now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Only one nitpick comment, and... should we also plan for some docs? It's not clear to me how this can be enabled, and what are the prerequisites.
Like There's only the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM! 👍
I've tried to test the package with http-client removed from dev-deps, but is still there.
Notably the Tests for |
We can either leave it like this, or remove |
I don't think we should change the CI to remove the package because we actually do require the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this is ready IMO! 👍 good job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @alekitto for the lovely work and the dedication to this contribution. LGTM!
@alekitto hey, I know it's been a while since you implemented this. While testing this feature, I noticed that correct span timing heavily relies on users calling I'm doing a bunch of "fire & forget" GET requests. You can see the next Did this pop up while working on this? |
Hi @cleptric, I don't remember anything similar, but also I don't remember if I tested this particular case. I don't know if this could be a viable approach... |
sentry-trace
header to requests to support distributed tracingDue to a recent change in symfony's
Container::getParameter
docblock, psalm php version has been increased to 8.1. Otherwiseunknown class \UnitEnum
errors would be emitted