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

Warnings when using Faraday adapter with Farday 0.17.3 #965

Closed
kitop opened this issue Mar 6, 2020 · 7 comments · Fixed by #971
Closed

Warnings when using Faraday adapter with Farday 0.17.3 #965

kitop opened this issue Mar 6, 2020 · 7 comments · Fixed by #971
Assignees
Labels
bug Involves a bug community Was opened by a community member integrations Involves tracing integrations
Projects
Milestone

Comments

@kitop
Copy link
Contributor

kitop commented Mar 6, 2020

When configuring the Faraday adapter globally with

Datadog.configure do |c|
   c.use :faraday
end

I start seeing this deprecation warning all around:

WARNING: Unexpected middleware set after the adapter. This won't be supported from Faraday 1.0.

Seems to come from the way it's patching Faraday::Connection here:
https://github.com/DataDog/dd-trace-rb/blob/v0.32.0/lib/ddtrace/contrib/faraday/patcher.rb#L42
https://github.com/DataDog/dd-trace-rb/blob/v0.32.0/lib/ddtrace/contrib/faraday/connection.rb

From this issue (lostisland/faraday#946) seems like there's not currently a way to add a default middleware that'll be supported in v1.0.

Is this a know issue? Are there any workarounds around it?

@delner
Copy link
Contributor

delner commented Mar 9, 2020

Hmmmm, thanks for the report @kitop !

If I'm understanding things correctly, sounds like Faraday's maintainers don't want default middleware because requests can be sensitive to the presence and order of the middleware stack. Although our APM middleware is unlikely to cause such issues, that's not universally true about other Faraday middleware, such that they'd rather just avoid the problem outright by preventing middleware from being added this way.

If there isn't some way of injecting this middleware at the base class level without raising this warning, then we might have to resort to some other form of aggressive monkey patching, perhaps something like lostisland/faraday#946 (comment)

We'll have to experiment... I'll know more when I find the time to replicate this in a test environment.

In the mean time, I think you should be able to avoid this by adding our middleware explicitly to the middleware stack when you declare your own Faraday connection (which should be in our documentation.)

@delner delner self-assigned this Mar 9, 2020
@delner delner added community Was opened by a community member integrations Involves tracing integrations bug Involves a bug labels Mar 9, 2020
@marcotc
Copy link
Member

marcotc commented Mar 9, 2020

I've opened #971 to address this warning.

@marcotc
Copy link
Member

marcotc commented Mar 10, 2020

👋 @kitop, our implementation currently works for both Faraday 0.x and 1.x, but version 0.x raises a warning due to the way we are patching Faraday.

We've changed the way we patch 0.x, moving back to the old strategy we used before #906 was merged, to allow it to run without warnings.

This warning does not affect correctness, so we'll ship it in our regular release cycle.

@kitop
Copy link
Contributor Author

kitop commented Mar 11, 2020

Hi @marcotc and @delner, thank you both for the quick response and turnaround!! Really appreciate it!

@rahul342
Copy link

rahul342 commented Mar 27, 2020

@delner @marcotc when is the next scheduled release? is it possible to release a patch version with #971. The issue is, it's overwhelming our logs, I could silence those temporarily but then we don't want to forget about it and have a breaking change some day.

@delner
Copy link
Contributor

delner commented Mar 27, 2020

@rahul342 I think we're aiming for a release early next week; we'll keep you posted.

@delner delner added this to the 0.34.0 milestone Mar 30, 2020
@delner
Copy link
Contributor

delner commented Mar 30, 2020

This will release with 0.34.0.

@delner delner added this to Resolved/Closed in Active work Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug community Was opened by a community member integrations Involves tracing integrations
Projects
Active work
  
Resolved/Closed
Development

Successfully merging a pull request may close this issue.

4 participants