-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add parsing for nested json fields in the rules sampler #418
Conversation
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.
I like the overall solution but I'm concerned with enabling JSON parsing of span data for all condition failures. This should be configuration driven to enable his behaviour.
sample/rules.go
Outdated
@@ -70,6 +72,16 @@ func (s *RulesBasedSampler) GetSampleRate(trace *types.Trace) (rate uint, keep b | |||
for _, span := range trace.GetSpans() { | |||
var match bool | |||
value, exists := span.Data[condition.Field] | |||
if !exists { |
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.
I'm concerned this would impact users that does not want to pay the cost of attempting to JSON parse span data when trying to resolve rule conditions.
I think we should look into adding an opt-in configuration option to enable this behaviour. Default would be disabled.
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.
That makes sense. I can add an optional parameter in the rules config file to enable it (similar to the AddSampleRateKeyToTrace and UseTraceLength fields), unless you think there is a better place for it.
I made the requested changes and tested with the refinery I have running with the nested fields parameter both enabled and disabled. The nested traces were correctly sampled when enabled and didn't affect normal sampling rules. |
Thanks @ecobrien29! We've had a busy couple weeks, but we'll take another look as soon as we can! |
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, thank you for the follow-up and sorry for the delay in getting back to you.
Which problem is this PR solving?
Adds support for checking nested JSON when validating rules. The option can be enabled using the
CheckNestedFields
configuration option.Short description of the changes