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: Remove trace key params -> add meta sample_key #685

Merged
merged 1 commit into from
May 18, 2023

Conversation

kentquirk
Copy link
Contributor

Which problem is this PR solving?

Refinery had rules configuration for dynamic samplers to "AddSampleRateKeyToTrace" which would inject the sample rate key into a field of the user's choice. We also have, in the normal configuration "AddRuleReasonToTrace", which also injected that key but as part of the rule reason in meta.refinery.reason, causing people to have to write derived columns to pull it off.

This PR addresses both of those.

Short description of the changes

  • Remove AddSampleRateKeyToTrace and AddSampleRateKeyToTraceField params from samplers
  • Remove all the tests for it
  • Modify the GetSampleRate function to return the reason and key separately (or a blank key)
  • If the key isn't blank, add it to the spans under meta.refinery.sample_key
  • Add tests for it

This looks big but it's a lot of small identical changes in multiple files.

@kentquirk kentquirk requested a review from a team as a code owner May 18, 2023 01:29
@kentquirk kentquirk changed the title Remove trace key params -> add meta sample_key feat: Remove trace key params -> add meta sample_key May 18, 2023
@kentquirk kentquirk added the breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major. label May 18, 2023
@kentquirk kentquirk added this to the v2.0 milestone May 18, 2023
@kentquirk kentquirk merged commit 494f6a5 into main May 18, 2023
@kentquirk kentquirk deleted the kent.v2.sample_key branch May 18, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Prefer 'version: bump major', but use this for breaking changes that don't bump major.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants