fix: add sort before hashing attributes #51
Conversation
src/scope.zig
Outdated
| test "InstrumentationScope hash should be consistent regardless of attribute order" { | ||
| const allocator = std.testing.allocator; | ||
|
|
||
| const attrs1 = try Attributes.from(allocator, .{ @as([]const u8, "key1"), @as(u64, 42), @as([]const u8, "key2"), true, @as([]const u8, "key3"), @as([]const u8, "value3") }); |
There was a problem hiding this comment.
Do we need type coercion with @as for the keys?
In other tests using Attributes we don't have to use it.
There was a problem hiding this comment.
True, adjusted this one to be similar to the other test cases.
| @@ -91,3 +91,21 @@ test "InstrumentationScope should equal correctly" { | |||
|
|
|||
| try std.testing.expect(hashContext.eql(first, second)); | |||
| } | |||
|
|
|||
| test "InstrumentationScope hash should be consistent regardless of attribute order" { | |||
There was a problem hiding this comment.
Thanks for adding the test 🙏🏼
src/attributes.zig
Outdated
| // If there are few attributes, just hash them directly to avoid allocation | ||
| if (attrs.len <= 8) { |
There was a problem hiding this comment.
This is an interesting approach.
Out of curiosity: why did you choose 8?
Is there a way to keep the whole sorting on the stack and never allocate?
The length of attributes is already known via self.attributes.len, so even reserving space on the stack for a sorted list should be doable?
There was a problem hiding this comment.
Oh yeah that was my first iteration of this one, and wasn't really familiar with the opentelemetry sdk specs. So i was just setting this to a low number.
Adjusted the implementation to handle this on the stack, and using now the max default as described here: https://opentelemetry.io/docs/specs/otel/common/#configurable-parameters
Previously, the hashing mechanism did not account for the order of attributes, leading to inconsistent hash values for the same set of attributes in different orders. This could cause issues when using
InstrumentationScopeas a key in hash maps, as equivalent scopes with different attribute orders would produce different hash values.Attributes.HashContext.hashto sort attributes by key before hashingTesting
Added a new test case
InstrumentationScope hash should be consistent regardless of attribute orderthat verifies:closes #48