api: proper cleanup of data, improve comments

inge4pres · inge4pres · commit 2df6d74f4fef · 2025-02-06T12:07:46.000+01:00
Signed-off-by: inge4pres &lt;fgualazzi@gmail.com&gt;
diff --git a/src/api/metrics/instrument.zig b/src/api/metrics/instrument.zig
@@ -68,6 +68,7 @@ pub const Instrument = struct {
     pub fn counter(self: *Self, comptime T: type) !*Counter(T) {
         const c = try self.allocator.create(Counter(T));
         c.* = Counter(T).init(self.allocator);
+        errdefer self.allocator.destroy(c);
         self.data = switch (T) {
             u16 => .{ .Counter_u16 = c },
             u32 => .{ .Counter_u32 = c },
@@ -83,6 +84,7 @@ pub const Instrument = struct {
     pub fn upDownCounter(self: *Self, comptime T: type) !*Counter(T) {
         const c = try self.allocator.create(Counter(T));
         c.* = Counter(T).init(self.allocator);
+        errdefer self.allocator.destroy(c);
         self.data = switch (T) {
             i16 => .{ .UpDownCounter_i16 = c },
             i32 => .{ .UpDownCounter_i32 = c },
@@ -98,6 +100,7 @@ pub const Instrument = struct {
     pub fn histogram(self: *Self, comptime T: type) !*Histogram(T) {
         const h = try self.allocator.create(Histogram(T));
         h.* = try Histogram(T).init(self.allocator, self.opts.histogramOpts);
+        errdefer self.allocator.destroy(h);
         self.data = switch (T) {
             u16 => .{ .Histogram_u16 = h },
             u32 => .{ .Histogram_u32 = h },
@@ -115,6 +118,7 @@ pub const Instrument = struct {
     pub fn gauge(self: *Self, comptime T: type) !*Gauge(T) {
         const g = try self.allocator.create(Gauge(T));
         g.* = Gauge(T).init(self.allocator);
+        errdefer self.allocator.destroy(g);
         self.data = switch (T) {
             i16 => .{ .Gauge_i16 = g },
             i32 => .{ .Gauge_i32 = g },
@@ -410,6 +414,14 @@ pub fn Gauge(comptime T: type) type {
 
 const MeterProvider = @import("meter.zig").MeterProvider;
 
+test "counter with unsupported type does not leak" {
+    const mp = try MeterProvider.default();
+    defer mp.shutdown();
+    const meter = try mp.getMeter(.{ .name = "my-meter" });
+    const err = meter.createCounter(u1, .{ .name = "a-counter" });
+    try std.testing.expectError(spec.FormatError.UnsupportedValueType, err);
+}
+
 test "meter can create counter instrument and record increase without attributes" {
     const mp = try MeterProvider.default();
     defer mp.shutdown();
@@ -570,8 +582,10 @@ test "instrument fetches measurements from inner" {
     defer std.testing.allocator.free(id);
 
     if (meter.instruments.get(id)) |instrument| {
-        var measurements = try instrument.getInstrumentsData(std.testing.allocator);
-        defer measurements.deinit(std.testing.allocator);
+        const measurements = try instrument.getInstrumentsData(std.testing.allocator);
+        defer switch (measurements) {
+            inline else => |list| std.testing.allocator.free(list),
+        };
 
         std.debug.assert(measurements.int.len == 1);
         try std.testing.expectEqual(@as(i64, 100), measurements.int[0].value);
diff --git a/src/api/metrics/measurement.zig b/src/api/metrics/measurement.zig
@@ -1,4 +1,6 @@
 const std = @import("std");
+const ArrayList = std.ArrayList;
+
 const Attribute = @import("../../attributes.zig").Attribute;
 const Attributes = @import("../../attributes.zig").Attributes;
 const Kind = @import("instrument.zig").Kind;
@@ -30,17 +32,10 @@ test "datapoint with attributes" {
 pub const MeasurementsData = union(enum) {
     int: []DataPoint(i64),
     double: []DataPoint(f64),
-
-    pub fn deinit(self: MeasurementsData, allocator: std.mem.Allocator) void {
-        switch (self) {
-            .int => allocator.free(self.int),
-            .double => allocator.free(self.double),
-        }
-    }
 };
 
 /// A set of data points with a series of metadata coming from the meter and the instrument.
-/// It holds the data collected by a single instrument inside a meter.
+/// Holds the data collected by a single instrument inside a meter.
 pub const Measurements = struct {
     meterName: []const u8,
     meterAttributes: ?[]Attribute = null,
@@ -52,6 +47,8 @@ pub const Measurements = struct {
     data: MeasurementsData,
 
     pub fn deinit(self: *Measurements, allocator: std.mem.Allocator) void {
-        self.data.deinit(allocator);
+        switch (self.data) {
+            inline else => |list| allocator.free(list),
+        }
     }
 };
diff --git a/src/api/metrics/meter.zig b/src/api/metrics/meter.zig
@@ -428,21 +428,31 @@ const view = @import("../../sdk/metrics/view.zig");
 pub const AggregatedMetrics = struct {
     fn deduplicate(allocator: std.mem.Allocator, instr: *Instrument, aggregation: view.Aggregation) !MeasurementsData {
         // This function is only called on read/export
-        // which is much less frequent than other SDK operations.
+        // which is much less frequent than other SDK operations (e.g. counter add).
         // TODO: update to @branchHint in 0.14+
         @setCold(true);
 
         const allMeasurements: MeasurementsData = try instr.getInstrumentsData(allocator);
-        defer allMeasurements.deinit(allocator);
+        defer {
+            switch (allMeasurements) {
+                inline else => |list| allocator.free(list),
+            }
+        }
 
         switch (allMeasurements) {
             .int => {
                 var deduped = std.ArrayList(DataPoint(i64)).init(allocator);
-                defer deduped.deinit();
 
-                var temp = std.HashMap(Attributes, i64, Attributes.HashContext, std.hash_map.default_max_load_percentage).init(allocator);
+                var temp = std.HashMap(
+                    Attributes,
+                    i64,
+                    Attributes.HashContext,
+                    std.hash_map.default_max_load_percentage,
+                ).init(allocator);
                 defer temp.deinit();
 
+                // iterate over all measurements and deduplicate them by applying the aggregation function
+                // using the attributes as the key.
                 for (allMeasurements.int) |measure| {
                     switch (aggregation) {
                         .Drop => return MeasurementsData{ .int = &[_]DataPoint(i64){} },
@@ -465,14 +475,12 @@ pub const AggregatedMetrics = struct {
 
                 var iter = temp.iterator();
                 while (iter.next()) |entry| {
-                    // TODO add sharedAttributes to the measurements attributes.
                     try deduped.append(DataPoint(i64){ .attributes = entry.key_ptr.*.attributes, .value = entry.value_ptr.* });
                 }
                 return .{ .int = try deduped.toOwnedSlice() };
             },
             .double => {
                 var deduped = std.ArrayList(DataPoint(f64)).init(allocator);
-                defer deduped.deinit();
 
                 var temp = std.AutoHashMap(Attributes, f64).init(allocator);
                 defer temp.deinit();
@@ -499,7 +507,6 @@ pub const AggregatedMetrics = struct {
 
                 var iter = temp.iterator();
                 while (iter.next()) |entry| {
-                    // TODO add sharedAttributes (the meter's attributes)to the measurements attributes.
                     try deduped.append(DataPoint(f64){ .attributes = entry.key_ptr.*.attributes, .value = entry.value_ptr.* });
                 }
                 return .{ .double = try deduped.toOwnedSlice() };
@@ -547,7 +554,9 @@ test "aggregated metrics deduplicated from meter without attributes" {
     const instr = iter.next() orelse unreachable;
 
     const deduped = try AggregatedMetrics.deduplicate(std.testing.allocator, instr.*, .Sum);
-    defer deduped.deinit(std.testing.allocator);
+    defer switch (deduped) {
+        inline else => |m| std.testing.allocator.free(m),
+    };
 
     try std.testing.expectEqualDeep(DataPoint(i64){ .value = 4 }, deduped.int[0]);
 }
@@ -570,8 +579,10 @@ test "aggregated metrics deduplicated from meter with attributes" {
     var iter = meter.instruments.valueIterator();
     const instr = iter.next() orelse unreachable;
 
-    var deduped = try AggregatedMetrics.deduplicate(std.testing.allocator, instr.*, .Sum);
-    defer deduped.deinit(std.testing.allocator);
+    const deduped = try AggregatedMetrics.deduplicate(std.testing.allocator, instr.*, .Sum);
+    defer switch (deduped) {
+        inline else => |m| std.testing.allocator.free(m),
+    };
 
     const attrs = try Attributes.from(std.testing.allocator, .{ "key", val });
     defer if (attrs) |a| std.testing.allocator.free(a);
