Fix HTTPTransport downgrade causing callback error #369
Conversation
lib/ddtrace/transport.rb
Outdated
| @@ -42,7 +46,7 @@ class HTTPTransport | |||
| private_constant :API | |||
|
|
|||
| def initialize(hostname, port, options = {}) | |||
| api_version = options.fetch(:api_version, V3) | |||
| api_version = options.fetch(:api_version, V4) | |||
There was a problem hiding this comment.
why the default is V4 now? V4 endpoint is for distributed sampling that is disabled by default. Endpoint V3 is the right one to use unless an explicit configuration is used.
There was a problem hiding this comment.
I think I misread the code here. It had looked like in writer.rb that the api_version was always being set to V4 anyways. That's only true of priority sampling though, not the default case.
I'll change this back.
delner
force-pushed
the
bugfix/transport_downgrade_callbacks
branch
from
902078d
to
b778d23
Compare
| service_rates = JSON.parse(response.body) | ||
| @priority_sampler.update(service_rates) | ||
|
|
||
| if api[:version] == HTTPTransport::V4 |
There was a problem hiding this comment.
that should do the trick, but I agree with you that this coupling between Writer and Transport makes the code brittle and should be addressed in a future PR.
| Datadog::Tracer.log.level = @original_level | ||
| end | ||
|
|
||
| describe '#handle_response' do |
There was a problem hiding this comment.
probably worth checking here if the callback is processed
There was a problem hiding this comment.
Seems reasonable. Although, for what it's worth, the callback is removed from this function in the next PR (becomes a part of post instead in the next PR.)
| subject(:result) { transport.handle_response(response) } | ||
|
|
||
| context 'given an OK response' do | ||
| let(:response) { Net::HTTPResponse.new(1.0, 200, 'OK') } |
There was a problem hiding this comment.
just a a thought: sometimes adding some randomness to the test might make it more robust without requiring a exhaustive list of test cases. in case of this method you could do something like:
let(:status_code) { (200...500).to_a.sample }
let(:response) { Net::HTTPResponse.new(1.0, status_code, 'whatever') }and test if the return value of the method is equal to status_code
There was a problem hiding this comment.
I think that works if the expected behavior is totally abstracted away from the input itself (e.g. it always replies with whatever input it was given.)
| end | ||
|
|
||
| describe '#send' do | ||
| before(:each) { skip "TEST_DATADOG_INTEGRATION not set." unless ENV['TEST_DATADOG_INTEGRATION'] } |
There was a problem hiding this comment.
perhaps we should use rspec tags for that?
| before(:each) do | ||
| original_post = transport.method(:post) | ||
| call_count = 0 | ||
| allow(transport).to receive(:post) do |url, *rest| |
| let(:body) { service_rates.to_json } | ||
|
|
||
| it do | ||
| expect(sampler).to receive(:update).with(service_rates) |
If priority sampling is enabled, and HTTPTransport triggered an API downgrade, for example after receiving a 404 from an outdated agent, it would pass the downgraded response to the callback expecting the new response format. This would consequently result in a parse error.
This pull request passes the API as a parameter to the callback, so it can use that information to change how it handles responses. This PR also adds tests for this scenario, after converting Minitests to RSpec to perform better mocking.
Side note; this should probably be considered a stop-gap measure. There's a lot of coupling between Writer, Transport, and the concept of "API" which makes this design fairly brittle. Would recommend a refactor in the future.