attribute: add SLICE type support#8166
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8166 +/- ##
=======================================
+ Coverage 82.3% 82.5% +0.1%
=======================================
Files 310 310
Lines 24267 24555 +288
=======================================
+ Hits 19990 20261 +271
- Misses 3901 3916 +15
- Partials 376 378 +2
🚀 New features to boost your workflow:
|
MrAlias
left a comment
There was a problem hiding this comment.
Overall, looks good.
I assume we're going to address the attribute limits and exporter support in a follow-up, right?
Correct: |
Replace reflect.ValueOf + reflection-based iteration with direct typed slice access (asBoolSlice, asInt64Slice, asFloat64Slice, asStringSlice) for BOOLSLICE, INT64SLICE, FLOAT64SLICE, and STRINGSLICE in hashKV. This avoids the per-element reflection overhead — the same improvement applied to the SLICE type in open-telemetry#8166. Fixes open-telemetry#8200 Signed-off-by: alliasgher <alliasgher123@gmail.com>
|
I am going to create a separate PR for improving the speed execution of EDIT: My idea is to change the test so that it only checks if |
Unblocks #8166 Per #8166 (comment) ## Why This updates `TestHashKVsEquality` to avoid the quadratic pairwise comparison that was causing slow runs and timeouts in CI. Below are the results from my machine. Old: ``` $ go test -run=TestHashKVs -count=1 PASS ok go.opentelemetry.io/otel/attribute 2.063s ``` New: ``` $ go test -run=TestHashKVs -count=1 PASS ok go.opentelemetry.io/otel/attribute 0.024s ``` Instead of collecting every generated testcase and comparing each hash against every other hash, the test now checks uniqueness as each case is generated by storing previously seen hashes in a map. Note that it also does not use the equality operator of `KeyValue` to determine if the hash should be equal or different. ## What - Rename the test from `TestHashKVsEquality` to `TestHashKVs` - Replace the O(n^2) post-processing loop with an O(n) streaming uniqueness check - Simplify failure reporting by removing the dedicated `msg` helper Side note: for me it also makes the test more readable (however, this is opinionated).
There was a problem hiding this comment.
Pull request overview
Adds first-class support for heterogeneous SLICE attribute values (arrays of attribute.Value) across the attribute package and downstream conversions, aligning with the OTel AnyValue array concept.
Changes:
- Introduce
attribute.SLICEplusSlice/SliceValueconstructors andValue.AsSlice()/string formatting support. - Propagate
SLICEhandling into trace auto-conversion, log attribute conversion, hashing, and metric test comparisons. - Add/extend unit tests, fuzz/bench coverage, and update changelog.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| trace/auto.go | Convert attribute.SLICE into telemetry nested slice values. |
| sdk/metric/metricdata/metricdatatest/comparisons.go | Extend attribute equality helper to handle SLICE. |
| log/keyvalue.go | Convert attribute.SLICE into log SliceValue recursively. |
| log/keyvalue_test.go | Add test coverage for SLICE conversion to log values. |
| log/keyvalue_bench_test.go | Add benchmark case for SLICE conversion. |
| attribute/value.go | Add SLICE type, constructors/accessors, formatting, and base64 writer helper. |
| attribute/value_test.go | Add tests for SLICE value behavior, AsSlice, and string formatting. |
| attribute/type_string.go | Regenerate Type.String() backing tables to include SLICE. |
| attribute/kv.go | Add attribute.Slice convenience constructor. |
| attribute/key.go | Add Key.Slice constructor. |
| attribute/kv_test.go | Add constructor/validity/incorrect-cast tests for SLICE. |
| attribute/key_test.go | Add Emit/String tests for SLICE formatting behavior. |
| attribute/hash.go | Add hashing support for SLICE values (including nested). |
| attribute/hash_test.go | Extend hash test generators + add SLICE fuzz/bench coverage. |
| attribute/benchmark_test.go | Add slice benchmarks for construction/access/string/emit. |
| CHANGELOG.md | Document new SLICE attribute type API additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #7934