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

Guzzle 6 Integration bug #185

Closed
inverse opened this issue Dec 12, 2018 · 9 comments
Closed

Guzzle 6 Integration bug #185

inverse opened this issue Dec 12, 2018 · 9 comments
Labels
🏘 community Contributions from the general community feature-request 🎉 new-integration A new integration

Comments

@inverse
Copy link
Contributor

inverse commented Dec 12, 2018

When integrating this into a project that uses guzzle 6.3 I get the following error

Attempted to call an undefined method named "getUrl" of class "GuzzleHttp\\Psr7\\Request".\nDid you mean to call "getUri"? 

I suspect that this was changed to getUri in 6. How would be best to add an integration in for that? Perhaps defining the dependency similar to what is done here for multiple versions of Symfony:

https://pantheon.io/blog/highest-lowest-testing-multiple-symfony-versions

@labbati
Copy link
Member

labbati commented Dec 12, 2018

Hi @inverse, thanks for reporting this issue. We currently support (and tests) 5.x but we were aware of 6.x version that had to be addresses! Let me get back to you soon with an ETA. In the meantime if you have free cycles we would more than welcome a PR 😄

@labbati labbati added 🏘 community Contributions from the general community 🎉 new-integration A new integration feature-request labels Dec 12, 2018
@inverse
Copy link
Contributor Author

inverse commented Dec 12, 2018

Totally would love to contribute to this! but not sure about free cycles right now.

Do you have a plan in mind how you would like to support multiple versions of libraries for example? I know that guzzle exposes a constant with a version but the way I saw you init is based on class only and not considering other factors.

Also for testing - I've not got any experience with this but have seen tools out there such as https://packagist.org/packages/g1a/composer-test-scenarios

Cheers for the feedback!

@labbati
Copy link
Member

labbati commented Dec 13, 2018

Hi @inverse

Do you have a plan in mind how you would like to support multiple versions of libraries for example? I know that guzzle exposes a constant with a version but the way I saw you init is based on class only and not considering other factors.

Proposals are welcomed of course, but this is the TL;DR version of what we were thinking to do:

  • Each integration, e.g. guzzle would have a file DDTrace\Integrations\Guzzle\GuzzleIntegration.php which exposes a single method ::load() to be called to load the integration (1)
  • That method tries to guess the version based of any info it can have access to, e.g. a version file vs a class that exists or not
  • Then it delegates the load() to the version-specific ...\Vx\GuzzleIntegration.php file (2)/(3).
DDTrace/
	Integrations/
		…
		Guzzle/
			V5/
				GuzzleIntegration.php (2)
			V6/
				GuzzleIntegration.php (3)
			GuzzleConfiguration.php  // Optional
			GuzzleIntegration.php (1)
		…
		IntegrationLoader.php
	Configuration.php

Also for testing - I've not got any experience with this but have seen tools out there such as https://packagist.org/packages/g1a/composer-test-scenarios

The link that you provided seems very promising, and in fact we were looking for something similar to what we use in python tracer and this library seems to be the answer. Very good suggestion I believe.

@labbati
Copy link
Member

labbati commented Dec 19, 2018

Hey @inverse, we may have some spare cycles to adopt the library you suggested in our tests. But I don't want to step over if you already did some work. Happy to merge/integrate in that case.
I played a few minutes with it and seems to be working very well.

@inverse
Copy link
Contributor Author

inverse commented Dec 19, 2018

Feel free to take it over :) I didn't get that far as I'm not able to work on this during work time and home life is another balancing act :) interested to see the implementation though :)

@labbati
Copy link
Member

labbati commented Dec 31, 2018

@inverse just an heads-up, if you are curious you can follow this PR #203 that introduces the test scenarios

@inverse
Copy link
Contributor Author

inverse commented Jan 1, 2019

thanks @labbati 👍

@labbati
Copy link
Member

labbati commented Feb 4, 2019

@inverse I am closing this issue as Guzzle 6 support has been added, finally 😄
Feel free to re-open it if you experience any other problem.
Thanks again for your advice on using https://packagist.org/packages/g1a/composer-test-scenarios it definitely helped moving forward in supporting (and testing) multiple versions of the same integration.

@labbati labbati closed this as completed Feb 4, 2019
@inverse
Copy link
Contributor Author

inverse commented Feb 4, 2019

@labbati that was quick! Will let the team involved in this know so that they can continue our integration.

bwoebi pushed a commit that referenced this issue Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏘 community Contributions from the general community feature-request 🎉 new-integration A new integration
Projects
None yet
Development

No branches or pull requests

2 participants