search

patrickkerrigan / php-xray

A PHP instrumentation library for AWS X-Ray
BSD 3-Clause "New" or "Revised" License
63 stars 26 forks source link

Fetch sampling rules from AWS console #3

Closed Ekman closed 3 years ago

Ekman commented 5 years ago

Thanks for the great effort! We're using this at the office since we're investing a lot in AWS, a shame that they don't support an official SDK.

The point with this PR is to implement sampling rules from the AWS console in order to advance this SDK one step further. The PR at this stage is not complete at all and it's just a quick first draft. I haven't really read the link I provided in very much detail. Right now, I want your acknowledgment that you think this is a good idea and that you would consider merging this. Also, I want your opinion on the general API and if you think it fits with the current API and your coding style. Any class or variable names are a first draft, in general I'm adaptable!

So the core of the new functionality is the SamplingRule interface which defines how to fetch the sampling rules, with a default implementation of fetching them using the AWS SDK. The point is to make as few assumptions as possible and therefore things such as the XrayClient are injected by whoever consumes the library using whatever preferred method they have. The CacheSamplingRule is there in order to provide a way to wrap any implementation with caching, you'd probably want that in the long run. The TraceManager currently provides an abstraction layer on top of submitting traces in order to customize each trace with information from the sampling rules. Not all functionality from sampling rules are implemented, just the ones that is dependent on the current request.

We could make any dependency a dev dependency, explaining to library consumers that they can require certain packages in order to unlock more features.

As soon as we start a dialog I'd be happy to make any changes and add tests for everything that has been added. Also, I'll double check the provided link that it matches the implementation that has been written.

patrickkerrigan commented 5 years ago

Thanks, nice to see that someone's getting some use from this library!

I'd been crossing my fingers for official PHP support with the latest round of additions to Xray, but as that doesn't seem to be coming any time soon I'm definitely up for merging this and getting the sampling rule functionality out there.

Like you, I haven't had the chance to read the full spec for sampling rules yet, but looking at this PR it fits with my current understanding.

The code and API look good so far to me; one of my goals with this was to keep it as loosely coupled as possible and let the consumer wire together what they need, which I think this fits well with. If possible I think it would be nice to keep the AWS SDK as an optional/suggested dependency to align with that, but let's see how feasible that looks when it's complete.

Code style-wise I've been trying to stick to PSR-2 with compatibility for PHP 7.0 and above.

Let me know if you need anything from me to get the last bits finished off, and thanks for the contribution!

Ekman commented 5 years ago

Awesome! I'll start working on it and I'll let you know if I have any questions or when it's ready for a real CR.

Ekman commented 5 years ago

Splitting the PR up in these tasks:

Ekman commented 5 years ago

Alright, I'm pretty happy with the PR in the current state. Let me know what you think! Looking forward to any comments, suggestions and/or criticism.

I'll be deploying this into one of our less critical services in order to test it as well.

patrickkerrigan commented 5 years ago

Looking great!

The only code comment I'd have so far is that it looks to me like matchesCriteria could be internal to SamplingRuleMatcher as it seems quite tightly coupled to the rest of the logic in that class.

It's also not too clear at the moment what would happen in the event the AWS API call fails. Maybe this could be configurable, e.g fallback to a set sampling rate?

I'd be interested to know how it performs in your internal testing. I'm looking forward to playing with sampling rules now!

Thanks again for your efforts

Ekman commented 5 years ago

Thank you!

You're right, I'll move matchesCriteria to the SamplingRuleMatcher. I like the idea of having a fallback, I'll add that.

I haven't deployed it yet unfortunately, had other prioritize to attend to. I'll be going on vacation for three weeks starting next week so I'll probably not be able to get back to you until august. Don't worry though, I'll not abandon this :) .

Ekman commented 5 years ago

I've added a SamplingRuleBuilder in order to make it easier for library consumers to know how to create sampling rules. I know I'd prefer libraries to do that instead of just accepting an array and documenting which keys it needs. WDYT?

patrickkerrigan commented 5 years ago

Looks good to me! Nice idea with the builder, it's much easier to use than expecting people to read the AWS docs.

Enjoy your time off!

austinheap commented 4 years ago

Is this going to get pulled in by chance? Great PR and library!

patrickkerrigan commented 4 years ago

Hi, thanks for your interest!

I'm definitely still interested in merging this PR provided that @Ekman is too.

Looking at where we left it, I think we were waiting to see how well the change performed in a test deployment. Given that this introduces blocking network calls that could potentially fail (and on every request at that), I'm cautious of merging until we can be reasonably sure that this library won't be the cause of anyone's production service being degraded.

The main area I'm interested in testing is the behaviour when the sampling rule cache is empty or expires. Under light load, this shouldn't be a problem, but under heavy load with high levels of concurrency I want to make sure we don't introduce a cache stampede.

@Ekman did you get a chance to make a test deployment in the end? If not, I can try and find some time to do some load testing on this PR to see how things behave during cache misses, and if any changes need making before this can be merged.

Ekman commented 4 years ago

Hi! Yes, I'm still very interested in this PR. I haven't got around to do any proper trusting in a real environment unfortunately. Anyone being able to help is greatly appreciated. Could also be awesome to get opinions on how using the new API is 🙂

Regarding cache stampede, my opinion is that it's up to the cache implementation to handle this. Since this PR is using a very widely adopted cache interface there's plenty of options to choose from, I'm sure they are handling it. If there's anything we could do from this side, I'm open to suggestions.

patrickkerrigan commented 4 years ago

That's a good point re: the caching libraries. I might grab a few of the most popular ones from packagist and hit them with a large number of concurrent requests to see how they handle it. If they take care of everything for us, then I think we should be good.

I'll let you know how I get on with the new API while integrating it into my test app!

Ekman commented 3 years ago

Sorry, haven't had the time to complete this.

We're moving away from AWS X-ray given it's lack of official PHP support. Therefore, I'll be closing this PR.