Skip to content
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

feat: use a computed field for current descendant count in rules #950

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Dec 14, 2023

Which problem is this PR solving?

Metadata about a trace is added only to root spans. However, it's useful to make sampling decisions before root span arrives. In this PR, I addressed a specific use case described in this issue

Short description of the changes

This PR adds a new concept called ComputedField in rule based sampler. A ComputedField is a virtual field that's generated during rule evaluation for a trace. The virtual field isn't actually defined on a span.

After some discussion with Kent, we decided to use ?. as the prefix for computed field so that it shouldn't collide with any regular field names.

This PR doesn't include documentation. I will address it in this PR for all rules related documentation.

@VinozzZ VinozzZ requested a review from a team as a code owner December 14, 2023 19:26
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good work. I had a really minor nit that you can ignore if you wish.

config/sampler_config.go Outdated Show resolved Hide resolved
sample/rules.go Show resolved Hide resolved
@VinozzZ VinozzZ force-pushed the yingrong.create_computed_field_on_traces branch from 70114af to d664e52 Compare December 14, 2023 21:47
@VinozzZ VinozzZ merged commit 637dd20 into main Dec 14, 2023
3 checks passed
@VinozzZ VinozzZ deleted the yingrong.create_computed_field_on_traces branch December 14, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants