-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(flags): capture feature flag evaluations on spans #16485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
03699e5
to
7c2c161
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one question: You need the buffer approach for determining the evaluation order right? Otherwise we could also just conditionally add the flag based on how many attributes are already present on the span.
Yes, I originally used buffers/arrays to track the eval order as we do in the scope/errors. However I don't believe this is a requirement as we're not enforcing order in Python spans, only recording the first 10 unique flags. IMO it's not as valuable for the span usecase, and for only 10 flags.
Yes that would also work as an alternative to the global weak map. Is there a way to do this without extending the |
@aliu39 something like if (shouldAddFeatureFlagToSpan(Sentry.spanToJSON(span).data, someFlag)) {
span.setAttribute(
// ...
)
} And in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to block you on this – we can also refactor this part in a follow up!
value: unknown, | ||
maxFlagsPerSpan: number = MAX_FLAGS_PER_SPAN, | ||
): void { | ||
const spanFlagMap = GLOBAL_OBJ._spanToFlagBufferMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there somewhere better to save this map, rather than globally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure - maybe the client? We can follow up, also with Charlie's proposal we wouldn't need the map anymore.
Based off https://github.com/getsentry/sentry-python/pull/4280/files
Reopened from #16475
This PR updates our 5 browser FF integrations to capture flag evaluations on spans. This implementation avoids changes to the
Span
class, saving bundle size.flag.evaluation.{flagKey}
From @AbhiPrasad :
From @cmanallen :