feat(spans): Record flag evaluations as span attributes

[cmanallen]

Flags evaluated within a span are appended to the span as attributes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.30%. Comparing base (6000f87) to head (0934e31).
Report is 19 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4280      +/-   ##
==========================================
+ Coverage   79.54%   80.30%   +0.75%     
==========================================
  Files         142      142              
  Lines       15907    15925      +18     
  Branches     2723     2724       +1     
==========================================
+ Hits        12654    12788     +134     
+ Misses       2389     2262     -127     
- Partials      864      875      +11     

Files with missing lines	Coverage Δ
sentry_sdk/feature_flags.py	82.85% <100.00%> (+1.60%)	⬆️
sentry_sdk/integrations/launchdarkly.py	77.77% <100.00%> (-0.61%)	⬇️
sentry_sdk/integrations/openfeature.py	69.56% <100.00%> (-2.44%)	⬇️
sentry_sdk/integrations/unleash.py	89.47% <100.00%> (-0.53%)	⬇️
sentry_sdk/tracing.py	78.44% <100.00%> (+0.82%)	⬆️

... and 9 files with indirect coverage changes

[cmanallen]

Or to simplify we could remove the span specific recording in each integration and on each call to start_span we extract the last n flags from the flags buffer. Then the integrations are only responsible for recording flags in the buffer. The SDK manages where they go.

But the down side of that is the flags buffer has no idea what span the flag was evaluated in. So it violates the contract that only flags evaluated within the span are appended to the span as attributes. So we'd need a second buffer which empties after every call to start_span. Which we might need anyway if we're going to cap the number of flag attributes we send on a span.

[cmanallen]

What facilities do we have for capping span attributes? Could a customer send a span with one million attributes today?

[indragiek]

@cmanallen We don't cap the # of span attributes today but we will likely cap a) the total size of the event b) the number of attributes or c) both - we haven't made a decision on this yet.

[szokeasaurusrex]

Generally lgtm, although I have some questions on some items which were unclear to me.

Also, prior to merging these changes, we should settle on semantic conventions for feature flag span data keys and add them to our develop docs and/or the semantic convention docs. I discussed the span conventions with @AbhiPrasad, it sounds like he has some ideas about how we should name span data keys, so you should discuss with him.

[szokeasaurusrex]

[question] why are we uninstalling the integration here? My guess is this would reduce the possibility of interactions between tests, but I this is my first time seeing this pattern in our tests so just want to check

[cmanallen]

This is how I found the test suite so I'm following the pattern. I'm not sure why its necessary or what the implications are if its removed. I could look into this more.

[szokeasaurusrex]

Interesting, perhaps I just haven't touched this part of the codebase – if that's the pattern you can disregard my comment. Maybe @antonpirker or @sentrivana would know why this is needed.

[cmanallen]

Now capping the number of flags to 10. No particular reason for that default. We can make it larger or smaller.

[cmanallen]

@szokeasaurusrex Did you have any more thoughts on this PR?

[szokeasaurusrex]

@cmanallen nothing to add, just please make sure to address my previous comment: #4280 (review)

Specifically to make sure the semantic convention is agreed upon and documented. @AbhiPrasad had some thoughts on this, I can put him as a reviewer

[cmanallen]

@szokeasaurusrex getsentry/sentry-conventions#42 getsentry/sentry-docs#13362

[szokeasaurusrex]

Awesome, hadn't seen those other conversations/PRs

[AbhiPrasad]

Exciting!