-
Notifications
You must be signed in to change notification settings - Fork 895
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
Rename built-in ExemplarFilters #2919
Rename built-in ExemplarFilters #2919
Conversation
@jsuereth PTAL. Thanks.! |
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.
LGTM with a changelog entry.
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 ok aiming for consistency, but two questions raised -
- Will this be confusing to users
- Should we be doing renames during feature-freeze? This causes churn / could break an SDK that has the feature implemented. Just want to check impact before making that change.
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 suggest to rename SampledWithTrace
to TraceBased
for consistency
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Reopening as it seems this was briefly forgotten, but has enough approvals to be merged. @cijothomas please consider addressing the latest bits so we can merge this ;) |
@reyang @jack-berg @jsuereth Please do a final check. "TraceBased" suggestion is done, and also added changelog. |
Done! |
Renames the built-in ExemplarFilters to AlwaysOn, AlwaysOff, TraceBased. (inspired from the built-in samplers in trace spec)
Minor clarification about ExemplarFilters, that they only make measurements eligible for being Exemplars.
Fix links:
Currently, the set of built-in ExemplarFilters are not documented in the SDK page, but user is taken to a link, which takes to yet another link, which lists the available ExemplarFilters. This PR adds the built-in filter details in the SDK page itself, instead of only being documented in the environment variable page.