[connector/spanmetricsconnector] add separate additional dimensions for calls and duration metrics #39134
Conversation
MovieStoreGuy
changed the title
|
@portertech Hi please review |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
wait PR #39664 |
| @@ -422,9 +438,8 @@ func (p *connectorImp) aggregateMetrics(traces ptrace.Traces) { | |||
|
|
|||
| rscAndEventAttrs.EnsureCapacity(resourceAttr.Len() + event.Attributes().Len()) | |||
| resourceAttr.CopyTo(rscAndEventAttrs) | |||
| // We cannot use CopyTo because it overrides the existing keys. | |||
There was a problem hiding this comment.
I think we should keep this comment because what cannot be done is to do event.Attributes().CopyTo(rscAdnEventAttrs)
There was a problem hiding this comment.
Note that event.Attributes().CopyTo(rscAdnEventAttrs) will overwrite the existing data in rscAdnEventAttrs.
On the other hand, using rscAndEventAttrs.PutStr(k, v.Str()) may lead to loss of type information due to v.Str().
A better alternative is to use v.CopyTo(rscAndEventAttrs.PutEmpty(k)), which preserves the type information, making the comment unnecessary.
For a similar use case:
There was a problem hiding this comment.
I agree with your comments and fine changing this from rscAndEventAttrs.PutStr to v.CopyTo(rscAndEventAttrs.PutEmpty(k)) (didn't know we can do this) I changed from event.Attributes().CopyTo(rscAdnEventAttrs) to the current code in my previous PR becuase of the issue you mentioned.
The reason why I would like to preserve the comment is because I don't want somebody in the future replace this:
event.Attributes().Range(func(k string, v pcommon.Value) bool {
v.CopyTo(rscAndEventAttrs.PutEmpty(k))
return true
})with this:
event.Attributes().CopyTo(rscAdnEventAttrs)and introduce a regression. Because it is not obvious that .CopyTo replaces evertyhing.
| @@ -422,9 +438,8 @@ func (p *connectorImp) aggregateMetrics(traces ptrace.Traces) { | |||
|
|
|||
| rscAndEventAttrs.EnsureCapacity(resourceAttr.Len() + event.Attributes().Len()) | |||
| resourceAttr.CopyTo(rscAndEventAttrs) | |||
| // We cannot use CopyTo because it overrides the existing keys. | |||
There was a problem hiding this comment.
I agree with your comments and fine changing this from rscAndEventAttrs.PutStr to v.CopyTo(rscAndEventAttrs.PutEmpty(k)) (didn't know we can do this) I changed from event.Attributes().CopyTo(rscAdnEventAttrs) to the current code in my previous PR becuase of the issue you mentioned.
The reason why I would like to preserve the comment is because I don't want somebody in the future replace this:
event.Attributes().Range(func(k string, v pcommon.Value) bool {
v.CopyTo(rscAndEventAttrs.PutEmpty(k))
return true
})with this:
event.Attributes().CopyTo(rscAdnEventAttrs)and introduce a regression. Because it is not obvious that .CopyTo replaces evertyhing.
|
@open-telemetry/collector-contrib-approvers can you merge this? |
…or calls and duration metrics (open-telemetry#39134) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description fix: open-telemetry#36805
…or calls and duration metrics (open-telemetry#39134) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description fix: open-telemetry#36805
…or calls and duration metrics (open-telemetry#39134) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description fix: open-telemetry#36805
Description
fix: #36805