search

openzipkin / zipkin-aws

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

Add tests for XrayUDPStorage / Reporter. #157

Closed anuraaga closed 4 years ago

anuraaga commented 4 years ago

I was randomly looking at the xray storage and want to convert to using Netty to likely improve performance a lot. But that would require benchmarking and reasoning about whether adding a dependency on Netty is OK (I guess so since other storage like elasticsearch has it).

In the meantime, it shouldn't require reasoning to use Netty for tests, which would help in the event we make a change to the storage implementation.

codefromthecrypt commented 4 years ago

main thing is this is used for a reporter which cant assume a netty dep. we could move the non netty code over there I suppose and split the two?

https://github.com/openzipkin/zipkin-aws/blob/master/reporter-xray-udp/pom.xml#L35

On Fri, Mar 6, 2020, 6:23 PM Anuraag Agrawal notifications@github.com wrote:

I was randomly looking at the xray storage and want to convert to using Netty to likely improve performance a lot. But that would require benchmarking and reasoning about whether adding a dependency on Netty is OK (I guess so since other storage like elasticsearch has it).

In the meantime, it shouldn't require reasoning to use Netty for tests, which would help in the event we make a change to the storage implementation.

You can view, comment on, or merge this pull request online at:

https://github.com/openzipkin/zipkin-aws/pull/157 Commit Summary

  • Add tests for XrayUDPStorage.

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/pull/157?email_source=notifications&email_token=AAAPVVZVHKC5J7LJCGLR5I3RGDFJXA5CNFSM4LC5ERE2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ITBZ4LA, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPVV6KLTTB4TV42XSR56LRGDFJXANCNFSM4LC5EREQ .

anuraaga commented 4 years ago

Yup did notice that and agree with splitting - it's a small amount of code to duplicate but with a good expectation for impact on the storage (non-reporter) side

codefromthecrypt commented 4 years ago

sgtm

On Fri, Mar 6, 2020, 6:28 PM Anuraag Agrawal notifications@github.com wrote:

Yup did notice that and agree with splitting - it's a small amount of code to duplicate but with a good expectation for impact on the storage (non-reporter) side

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/openzipkin/zipkin-aws/pull/157?email_source=notifications&email_token=AAAPVVZFDPD7DMM64EGKVO3RGDF4HA5CNFSM4LC5ERE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOA3Z2A#issuecomment-595705064, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAPVVYAQ2DQ5HVPCDO3GFDRGDF4HANCNFSM4LC5EREQ .

anuraaga commented 4 years ago

Added tests for reporter too in anticipation of the split.