-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[connector/spanmetrics] disable exemplars by default #24048
[connector/spanmetrics] disable exemplars by default #24048
Conversation
@@ -55,6 +55,9 @@ type Config struct { | |||
|
|||
// Namespace is the namespace of the metrics emitted by the connector. | |||
Namespace string `mapstructure:"namespace"` | |||
|
|||
// ExemplarConfig defines the configuration for exemplars. | |||
ExemplarConfig ExemplarConfig `mapstructure:"exemplar"` |
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.
ExemplarConfig
is already within the Config
type. What do you think about renaming the field and type to just Exemplars
?
There seems to be a mix of plural exemplars
and singular exemplar
in this PR. Which should it be? I vote for exemplars
, but don't feel too strongly about it if folks prefer the singular variant.
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.
Updated to exemplars
# exemplars disabled | ||
spanmetrics/exemplars_disabled: | ||
exemplars: | ||
enabled: true |
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.
The comment and name suggests exemplars are disabled, but it's enabled here.
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.
renamed to exemplars_enabled, thanks for noticing!
Thanks for review @albertteoh, updated based on your comments, ready for round two :) |
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
@povilasv can you rebase or merge main? We fixed some issues on CI that are showing up on this PR |
Co-authored-by: Albert <[email protected]>
Co-authored-by: Pablo Baeyens <[email protected]>
5c0a399
to
334e1a3
Compare
Description:
Breaking change! Allows enabling / disabling Exemplars.
Link to tracking Issue:
#23872
Testing:
Documentation: