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

Tracing: symfony http client #606

Merged
merged 31 commits into from Jul 14, 2022
Merged

Tracing: symfony http client #606

merged 31 commits into from Jul 14, 2022

Conversation

alekitto
Copy link
Contributor

  • Add tracing to symfony http client
  • Add sentry-trace header to requests to support distributed tracing

Due to a recent change in symfony's Container::getParameter docblock, psalm php version has been increased to 8.1. Otherwise unknown class \UnitEnum errors would be emitted

Copy link
Collaborator

@ste93cry ste93cry left a 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 🥳

src/Tracing/HttpClient/TraceableHttpClientForV6.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/TraceableResponseForV4.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/TraceableResponseForV5.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/TraceableResponseForV6.php Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@ste93cry ste93cry left a 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)

src/DependencyInjection/SentryExtension.php Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
src/Tracing/HttpClient/AbstractTraceableResponse.php Outdated Show resolved Hide resolved
@ste93cry ste93cry deleted the branch getsentry:develop May 30, 2022
@ste93cry ste93cry closed this May 30, 2022
@alekitto
Copy link
Contributor Author

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

@ste93cry
Copy link
Collaborator

Sorry, I did not meant to close it. I mistakenly deleted the develop branch while merging another PR and this one got closed as a consequence

@ste93cry ste93cry reopened this May 30, 2022
@ste93cry ste93cry modified the milestones: 4.3, 4.4 May 30, 2022
@github-actions

This comment was marked as outdated.

@alekitto alekitto force-pushed the master branch 2 times, most recently from 68a2963 to 61b7a5e Compare Jun 22, 2022
@alekitto
Copy link
Contributor Author

I've added some tests and rebased the branch. All issues should be resolved now.

Copy link
Collaborator

@Jean85 Jean85 left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
@alekitto
Copy link
Contributor Author

Like dbal, twig and cache tracing, this is enabled by default if the component is installed (=> if class Symfony\Component\HttpClient\HttpClient exists).

There's only the enabled configuration option, so the only prerequisite is the symfony/http-client component to be installed

Jean85
Jean85 approved these changes Jun 29, 2022
Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Great! LGTM! 👍

@alekitto
Copy link
Contributor Author

alekitto commented Jul 5, 2022

  • the exception when tracing gets enabled but the package is missing (for which we have a dedicated CI job already)

I've tried to test the package with http-client removed from dev-deps, but is still there. composer why symfony/http-client outputs:

sentry/sdk                3.2.0    requires   symfony/http-client (^4.3|^5.0|^6.0)  
symfony/framework-bundle  v5.4.10  conflicts  symfony/http-client (<4.4)            
symfony/http-kernel       v5.4.10  conflicts  symfony/http-client (<5.0)            

Notably the sentry/sdk package requires symfony/http-client and cannot be removed.
The test expecting the exception is present, but I really don't know how to invoke it 😅

Tests for stream method and withOptions are now OK

@Jean85
Copy link
Collaborator

Jean85 commented Jul 5, 2022

sentry/sdk is just a package that auto-suggest the default for having a client implementation.

We can either leave it like this, or remove sentry/sdk in the CI to test that part. @ste93cry WDYT?

@ste93cry
Copy link
Collaborator

ste93cry commented Jul 5, 2022

I don't think we should change the CI to remove the package because we actually do require the symfony/http-client (even though indirectly), so the test doesn't really make sense in the context of what would happen in the real world.

Jean85
Jean85 approved these changes Jul 6, 2022
Copy link
Collaborator

@Jean85 Jean85 left a 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!

@Jean85 Jean85 requested a review from ste93cry Jul 6, 2022
Copy link
Collaborator

@ste93cry ste93cry left a 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!

@ste93cry ste93cry merged commit 0c6eee6 into getsentry:develop Jul 14, 2022
24 checks passed
@alekitto
Copy link
Contributor Author

Thanks @ste93cry and @Jean85 for the support and continuous review of this PR (and obviously for all the work on this bundle) :)

@cleptric
Copy link
Member

@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 $response->getContent() or (array) $response.

Screenshot 2022-11-24 at 14 01 39

I'm doing a bunch of "fire & forget" GET requests. You can see the next cache span starts before the last http.client span is finished, as the span is finished once __deconstruct is called, which is obviously too late.

Did this pop up while working on this?

@alekitto
Copy link
Contributor Author

Hi @cleptric,
Sorry, I only saw your comment now.

I don't remember anything similar, but also I don't remember if I tested this particular case.
Anyway, reading the issue on symfony repository, it seems that the API design makes impossible to know exactly when the request finished without a method call; however, an on_progress callback could be added to the options to check if the request has been completed and stop the Span.

I don't know if this could be a viable approach...

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.

None yet

4 participants