Skip to content

fix: add sort before hashing attributes #51

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

Merged
merged 3 commits into from
May 18, 2025
Merged

fix: add sort before hashing attributes #51

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
346480b
fix: add sort before hashing attributes
hendriknielaender May 16, 2025
bcacdd0
fix: remove type coercion in test
hendriknielaender May 17, 2025
15460c7
fix: adjust attributes handling
hendriknielaender May 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 37 additions & 5 deletions src/attributes.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,46 @@ pub const Attributes = struct {
}

pub const HashContext = struct {
pub fn hash(_: HashContext, self: Self) u64 {
fn compareAttributes(context: void, a: Attribute, b: Attribute) bool {
_ = context;
return std.mem.lessThan(u8, a.key, b.key);
}

pub fn hash(_: HashContext, self: Self) !u64 {
var h = std.hash.Wyhash.init(0);
const attrs = self.attributes orelse &[_]Attribute{};
for (attrs) |attr| {
h.update(attr.key);
h.update(attr.value.toStringNoAlloc());

// If there are few attributes, just hash them directly to avoid allocation
if (attrs.len <= 8) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var buffer: [8]Attribute = undefined;
const to_sort = buffer[0..@min(attrs.len, 8)];
@memcpy(to_sort, attrs[0..to_sort.len]);

if (to_sort.len > 1) {
std.mem.sort(Attribute, to_sort, {}, compareAttributes);
}

for (to_sort) |attr| {
h.update(attr.key);
h.update(attr.value.toStringNoAlloc());
}
return h.final();
} else {
var arena = std.heap.ArenaAllocator.init(std.heap.page_allocator);
defer arena.deinit();

const allocator = arena.allocator();
const sorted_attrs = try allocator.dupe(Attribute, attrs);
if (sorted_attrs.len > 1) {
std.mem.sort(Attribute, sorted_attrs, {}, compareAttributes);
}

for (sorted_attrs) |attr| {
h.update(attr.key);
h.update(attr.value.toStringNoAlloc());
}
return h.final();
}
return h.final();
}
pub fn eql(_: HashContext, a: Self, b: Self) bool {
const aAttrs = a.attributes orelse &[_]Attribute{};
Expand Down
20 changes: 19 additions & 1 deletion src/scope.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub const InstrumentationScope = struct {
pub fn hash(_: HashContext, self: Self) u64 {
const hashContext = Attributes.HashContext{};

const attributesHash = hashContext.hash(Attributes.with(self.attributes));
const attributesHash = hashContext.hash(Attributes.with(self.attributes)) catch @panic("Failed to hash attributes");

var h = std.hash.Wyhash.init(0);

Expand Down Expand Up @@ -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" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test 🙏🏼

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") });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need type coercion with @as for the keys?
In other tests using Attributes we don't have to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, adjusted this one to be similar to the other test cases.

defer allocator.free(attrs1.?);

const attrs2 = try Attributes.from(allocator, .{ @as([]const u8, "key3"), @as([]const u8, "value3"), @as([]const u8, "key1"), @as(u64, 42), @as([]const u8, "key2"), true });
defer allocator.free(attrs2.?);

const scope1: InstrumentationScope = .{ .name = "testScope", .attributes = attrs1 };
const scope2: InstrumentationScope = .{ .name = "testScope", .attributes = attrs2 };

const hashContext = InstrumentationScope.HashContext{};

try std.testing.expectEqual(hashContext.hash(scope1), hashContext.hash(scope2));
try std.testing.expect(hashContext.eql(scope1, scope2));
}
Loading