search

nipwaayoni / elastic-apm-php-agent

PHP Agent for Elastic APM
Other
28 stars 15 forks source link

sampleEvent() should give the same result for all calls in the transaction #70

Closed cykirsch closed 3 years ago

cykirsch commented 3 years ago

https://github.com/nipwaayoni/elastic-apm-php-agent/blob/5116a805f6fab965317d618e8cfdec02e833bfeb/src/Events/TransactionSampleStrategy.php#L20

In my application I set the rate to 0.1 and saw lots and lots of transactions that only show the top level and no spans, or show the top level and an odd collection of spans.

I think I narrowed it down to the sampling strategy. The linked code above is the logic that determines whether a transaction or span is included. But it shouldn't pick and choose spans within a transaction; rather, if the transaction is to be included, it should return all spans for that transaction.

I see two paths forward--either ensure that this function is only called once for a transaction and then only its result is used, or ensure that any time it is called from a given transaction its result is the same.

dstepe commented 3 years ago

That's a pretty obvious issue once it's been pointed out. I think the correct solution is to have the Transaction use the strategy to generate an isSampled value for the transaction itself. That should be an easy fix. I should be able to get a patch out in the next 24 hours.