[ENHANCEMENT] Use resource attributes for multiplying span metrics #4210
Conversation
…r_key to multiplier_key
JunhoeKim
requested review from
knylander-grafana,
joe-elliott,
annanay25,
mdisibio,
mapno,
yvrhdn,
zalegrala,
electron0zero,
ie-pham,
stoewer and
javiermolinar
as code owners
|
|
CHANGELOG.md
Outdated
| @@ -1,5 +1,5 @@ | |||
| ## main / unreleased | |||
|
|
|||
| * [ENHANCEMENT] The property previously named span_multiplier_key is now renamed to multiplier_key, utilizing the resource attributes. [#4210](https://github.com/grafana/tempo/pull/4210) | |||
There was a problem hiding this comment.
Should this be marked as a breaking change since the configuration name is changing from span_multiplier_key to multiplier_key? We'll need to make sure that we add this to the upgrade notes for the next release.
| if kv.Key == ratioKey { | ||
| v := kv.Value.GetDoubleValue() | ||
| if v > 0 { | ||
| spanMultiplier = 1.0 / v | ||
| multiplier = 1.0 / v |
There was a problem hiding this comment.
we should return after we find the appropriate value. no reason to keep iterating. i'm not sure why the original code didn't return.
| if ratioKey != "" { | ||
| for _, kv := range span.Attributes { | ||
| for _, kv := range append(span.Attributes, rs.Attributes...) { |
There was a problem hiding this comment.
this append will unnecessarily alloc. it would be preferred to just iterate each attribute slice separately.
| # Attribute Key to multiply span metrics | ||
| [span_multiplier_key: <string> | default = ""] | ||
| # Attribute Key to multiply metrics | ||
| [multiplier_key: <string> | default = ""] |
There was a problem hiding this comment.
we can change this config name, but we need to mark it a breaking change. i don't see a particularly strong reason to change it. the behavior has changed some, but it still is a "multiplier for span metrics".
| @@ -412,8 +412,8 @@ metrics_generator: | |||
| # Enable traces_target_info metrics | |||
| [enable_target_info: <bool> | default = false] | |||
|
|
|||
| # Attribute Key to multiply span metrics | |||
| [span_multiplier_key: <string> | default = ""] | |||
| # Attribute Key to multiply metrics | |||
There was a problem hiding this comment.
we should indicate that the attribute name is searched for in both resource and span level attributes
|
@knylander-grafana @joe-elliott |
…4210) * feat: Use resource attribute too, rename property from span_multiplier_key to multiplier_key * chore: update CHANGELOG.md * fix: apply review * fix: minor
What this PR does:
The property previously named span_multiplier_key is now renamed to multiplier_key, utilizing the resource attributes.
Which issue(s) this PR fixes:
Fixes #4202
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]