-
Notifications
You must be signed in to change notification settings - Fork 33
Tweak rule based sampler #461
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
base: main
Are you sure you want to change the base?
Conversation
| - array | ||
| - 'null' | ||
| type: array | ||
| minItems: 1 |
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.
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.
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.
Initially I thought it could be fine to allow empty rules, for example if the config is programatically generated based on something else but ends up with []. Of course, such a generator could be sophisticated enough to swap in always_off for such a scenario so ok with either way.
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.
Of course, such a generator could be sophisticated enough to swap in always_off
Exactly. Given that we can always loosen constraints but not add them, I'd rather start stricter and loosen in response to feedback.
| The rules for the sampler, matched in order. | ||
| Each rule can have multiple match conditions. All conditions must match for the rule to match. | ||
| If no conditions are specified, the rule matches all spans that reach it. | ||
| If no rules match, the span is not sampled. |
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.
This is documented in kitchen-sink.yaml but this behavior is important to both implementers and users and so should be part of the schema via description.
| } | ||
| } | ||
| }, | ||
| "ExperimentalComposableRuleBasedSamplerRule": { |
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.
Thanks for the help @jack-berg! I agree with your open-telemetry/opentelemetry-java#7861 (comment) missed that case when reasoning out the use cases. Should we have a predicates array of these that are ANDed?
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.
👍 tracking in #464


Nothing important for rc3.