search

openzipkin / zipkin-aws

Reporters and collectors for use in Amazon's cloud
Apache License 2.0
69 stars 34 forks source link

AWS SDK v2 interceptor #112

Closed devinsba closed 5 years ago

devinsba commented 5 years ago

I'm surprised the tests are passing. I have 3 failing tests when running locally....

I wonder if MockWebServer has slightly different behavior on MacOS, or maybe Mojave changed something. I don't know. I guess if this passes on Circle then it is ready for review.

codefromthecrypt commented 5 years ago

@devinsba lemme know when you'd like a look

devinsba commented 5 years ago

Review whenever. I'll try to find the issue with the tests on MacOS out of band and fix it in both libraries

devinsba commented 5 years ago

So I narrowed the failure I am seeing on tests to only when run in IntelliJ so I think we can move forward. I'll drop a couple comments.

screen shot 2018-11-23 at 3 56 23 pm
devinsba commented 5 years ago

@adriancole I didn't use scoping as I just followed the pattern from the previous implementation. Though we did have one addition in that impl of the thread pool wrapper. I'll take another shot at it now that the PendingSpans change is in. I was seeing some odd behavior on that front

llinder commented 5 years ago

Finally taking a look at this and don't have anything specific to add that hasn't already been commented other than it looks great!