fix(perf-issues): Use parameterized URL for similarities on N+1 API Calls

[leeandher]

Sanity check here, but I believe these parameterized URLs are already being used for fingerprinting, but they are not being used for checking similarity. Instead we are checking hashes from span_hashes which is generated from a pinned function which does not attempt to parameterize, meaning the hash of /posts/1/comments/ is not going to match /posts/2/comments/.

Rather than hashing the URLs, we can compare them directly after parameterization, in order to increase how often we can fingerprint. Added tests for this new behaviour and adjusted some previous ones.

As per the recommendation, I think I'm going to have this shadow for a while before actually implementing this to ensure we don't cause any fingerprint explosions.

This will allow for N+1 API calls to trigger when we process a transaction that does something like:

for (let i = 0; i < 20; i++) { 
  await fetch(`https://example.com/posts/${i}/`)
}

rather than just for query params like:

for (let i = 0; i < 20; i++) { 
  await fetch(`https://example.com/posts/?id=${i}`)
}

[codecov]

Codecov Report

Attention: Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
..._issues/detectors/n_plus_one_api_calls_detector.py	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #89362      +/-   ##
==========================================
- Coverage   85.26%   85.26%   -0.01%     
==========================================
  Files       10172    10172              
  Lines      574286   574301      +15     
  Branches    22636    22636              
==========================================
+ Hits       489671   489680       +9     
- Misses      84070    84076       +6     
  Partials      545      545              

[gggritso]

🤔 This looks pretty good to me! Interesting find, for sure.

I don't remember why the span similarity is done using hashes rather than parameterized URLs. I'm guessing it's something that slipped through the cracks in #43506 and I never noticed. Comparing the parameterized URLs seems okay to me, it's just hard to predict exactly what'll happen, and I'm paranoid. I think this is good?

re: span["hash"]. Here is how/when they're set:

sentry/src/sentry/event_manager.py

Line 2451 in 321dc2c

groupings.write_to_event(event.data)

I think you'd have to be pretty careful about changing those. N+1 API Calls won't be affected since it makes its own hashes for fingerprints, but other detectors might rely on span["hash"] for fingerprints, and if the hashes change everyone's going to get new issues. Should be easy to check, though!

These span hashes are also used for the "suspect spans" feature, though I don't know if that feature is going to stick around

Lastly, I'm not sure if you're worried about rolling this change out (I'm not sure if you need to be, but maybe?) but there's always a risk of creating a bunch of unwanted issues for people.

If you want to be rigorous, what we've done in the past is copy/paste a detector, register it, run it silently for a while in parallel with the original and check the output via telemetry. If the output looks good, then you can add those changes to the current detector.

I'm not saying this PR needs that, but it's worth considering.

[leeandher]

Closing in favour of #90070