Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Equal() helper method to pcommon.Value #12561

Closed
dmathieu opened this issue Mar 5, 2025 · 7 comments · Fixed by #12594
Closed

Add Equal() helper method to pcommon.Value #12561

dmathieu opened this issue Mar 5, 2025 · 7 comments · Fixed by #12594
Assignees
@dmathieu

Comments

@dmathieu
Copy link
Member

dmathieu commented Mar 5, 2025 •
edited
Loading 

          nit: I know that `AsRaw()` could make a copy of the data. This is a reminder that it would be nice if you could write `a.Value().Equal(value)` and avoid the allocation.

Originally posted by @jmacd in #12390 (comment)

So we don't need to use AsRaw to do value comparison.
@dmathieu dmathieu mentioned this issue Mar 5, 2025
Merged
@dmathieu dmathieu self-assigned this Mar 5, 2025
@dmathieu dmathieu mentioned this issue Mar 5, 2025
Closed
@bogdandrutu
Copy link
Member

The signature of the "AddAttribute" is not respecting our standard, pleas consider, either one of the options:

  1. Rename AddAttributes to AddRawAttribute
  2. Replace value any with value Value

@dmathieu
Copy link
Member Author

dmathieu commented Mar 7, 2025

Which one would you prefer?

@jmacd
Copy link
Contributor

jmacd commented Mar 7, 2025

I prefer the second option, value Value.

@bogdandrutu
Copy link
Member

I do also prefer the second option.

@bogdandrutu
Copy link
Member

We still need the Equal but they will accept the same type.

@jmacd
Copy link
Contributor

jmacd commented Mar 7, 2025

Wait a second -- the original comment of mine was to point out that Equal() could avoid allocations. If the user has to create a value object to perform the comparison, won't they have the same copy? The problem is that to construct a value w/ any of the complex types, you have to copy for safety, right (e.g., []byte values, slice values, map values).

@dmathieu dmathieu mentioned this issue Mar 10, 2025
Merged
@bogdandrutu
Copy link
Member

@jmacd my comment was to change AddAttribute to accept Value and not any.

@dmathieu dmathieu mentioned this issue Mar 11, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 12, 2025
@dmathieu
Introduce Equal comparison in pcommon types (#12594)
37e38f9 
#### Description

This introduces `.Equal()` methods to most pcommon types, so equality
comparison can be performed with no allocations.

The types `Equal` is added to are:

* `Value`
* `ByteSlice`
* `Float64Slice`
* `Int32Slice`
* `Int64Slice`
* `StringSlice`
* `Uint64Slice`
* `Map`
* `Slice`

The original intent was to add it to `Value`. However, to be able to
handle every type in there, I had to also add it to the other types,
some of which are coming too because they are auto-generated.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #12561

This is the same thing as #12568, but with comparison of pcommon types
rather than raw ones.

<!--Describe what testing was performed and which tests were added.-->
#### Testing

Here are the results from the new benchmarks.

```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/pdata/pcommon
cpu: Apple M1 Max
BenchmarkByteSliceEqual-10              513754251                2.181 ns/op           0 B/op          0 allocs/op
BenchmarkFloat64SliceEqual-10           550760200                2.189 ns/op           0 B/op          0 allocs/op
BenchmarkInt32SliceEqual-10             550836993                2.186 ns/op           0 B/op          0 allocs/op
BenchmarkInt64SliceEqual-10             536615931                2.178 ns/op           0 B/op          0 allocs/op
BenchmarkStringSliceEqual-10            166586130                7.055 ns/op           0 B/op          0 allocs/op
BenchmarkUInt64SliceEqual-10            550221884                2.191 ns/op           0 B/op          0 allocs/op
BenchmarkMapEqual-10                    100000000               10.29 ns/op            0 B/op          0 allocs/op
BenchmarkSliceEqual-10                  165937951                7.185 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/nil-10              425391500                2.817 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/strings-10          226853427                5.441 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/booleans-10         296628630                4.080 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/ints-10             296841543                4.070 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/doubles-10          296242837                4.050 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/byte_slices-10              173515455                6.855 ns/op           0 B/op          0 allocs/op
BenchmarkValueEqual/slices-10                   83512392                14.10 ns/op            0 B/op          0 allocs/op
BenchmarkValueEqual/maps-10                     63556654                18.41 ns/op            0 B/op          0 allocs/op
PASS
ok      go.opentelemetry.io/collector/pdata/pcommon     24.650s
```
github-merge-queue bot pushed a commit that referenced this issue Mar 13, 2025
@dmathieu
Switch the pprofile.AddAttribute to use pcommon.Value rather than raw…
f9f15c6 
… data (#12608)

#### Description

As discussed in
#12561,
this switches the `pprofile.AddAttribute` method to use `pcommon.Value`
rather than raw data.

I didn't add a changelog entry, because this method has not been
released yet. So the [introduction changelog
entry](https://github.com/open-telemetry/opentelemetry-collector/blob/main/.chloggen/pprofile-add-attribute.yaml)
should be enough.


#### Testing

Benchmark tests:

```
goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/collector/pdata/pprofile
cpu: Apple M1 Max
BenchmarkAddAttribute/with_a_new_string_attribute-10            47255384                25.08 ns/op           16 B/op          1 allocs/op
BenchmarkAddAttribute/with_an_existing_attribute-10             46872556                25.16 ns/op           16 B/op          1 allocs/op
BenchmarkAddAttribute/with_a_duplicate_attribute-10             47020178                25.18 ns/op           16 B/op          1 allocs/op
BenchmarkAddAttribute/with_a_hundred_attributes_to_loop_through-10               9166004               131.2 ns/op            16 B/op          1 allocs/op
PASS
ok      go.opentelemetry.io/collector/pdata/pprofile    6.246s
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmathieu dmathieu

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@dmathieu @bogdandrutu @jmacd