Metrics: implement OTLP and its exporter #41
Conversation
inge4pres
force-pushed
the
metrics/stable-exporters-2
branch
from
91239bd to
903e852
Compare
| @@ -90,13 +90,15 @@ test "exporters/in_memory" { | |||
|
|
|||
| try underTest.append(allocator, Measurements{ | |||
| .meterName = "first-meter", | |||
| .meterVersion = "1.0", | |||
There was a problem hiding this comment.
This will be made unnecssary once we work on #38
| @@ -18,7 +19,94 @@ const ManagedString = protobuf.ManagedString; | |||
| const pbcommon = @import("../../../opentelemetry/proto/common/v1.pb.zig"); | |||
| const pbmetrics = @import("../../../opentelemetry/proto/metrics/v1.pb.zig"); | |||
|
|
|||
| pub fn toProtobufMetric( | |||
There was a problem hiding this comment.
This file used to be a helper for converting the internal metrics data model to the protobuf types.
It is now moved to an implementation of an exporter (see where we call it in sdk/metrics/exporters.zig).
| var a = try std.ArrayList(pbmetrics.NumberDataPoint).initCapacity(allocator, data_points.len); | ||
| for (data_points) |dp| { | ||
| const attrs = try attributesToProtobufKeyValueList(allocator, dp.attributes); | ||
| const number_dp = pbmetrics.NumberDataPoint{ | ||
| a.appendAssumeCapacity(pbmetrics.NumberDataPoint{ |
There was a problem hiding this comment.
Small perf improvement: save allocations as we already know the number of items for the list.
| try std.testing.expectEqual(payload.len, 0); | ||
| } | ||
|
|
||
| /// Configuration options for the OTLP transport. |
There was a problem hiding this comment.
| // Implements the Exponential Backoff Retry strategy for HTTP requests. | ||
| // The default configuration values are not dictated by the OTLP spec. | ||
| // We choose toset a maxumum number of retries, even if not specified in the spec, | ||
| // to avoid infinite loops in case of a un-responsive server. | ||
| const ExpBackoffRetry = struct { | ||
| const Self = @This(); | ||
|
|
||
| allocator: std.mem.Allocator, | ||
| client: *http.Client, | ||
|
|
||
| max_retries: u32, | ||
| base_delay_ms: u64, | ||
| max_delay_ms: u64, | ||
|
|
||
| queue: std.PriorityQueue(FetchRequest, void, compareFetchRequest), | ||
| thread: ?std.Thread, | ||
| mutex: std.Thread.Mutex, | ||
| condition: std.Thread.Condition, | ||
| running: std.atomic.Value(bool), |
There was a problem hiding this comment.
I tried this to have a more structured approach to retries.
Turns out that it's too complex, and I'm ditching it in #47 tosimply fire off a new thread for each request that needs to be retried.
Alas, you don't have to review this part.
| while (self.running.load(.acquire)) { | ||
| var req: FetchRequest = undefined; |
There was a problem hiding this comment.
There's a deadlock here: since we .acquire the atomic in each loop iteration, but never release it, it only does the first retry and blocks forever.
| const pbtrace = @import("opentelemetry/proto/trace/v1.pb.zig"); | ||
|
|
||
| /// Export the data to the OTLP endpoint using the options configured with ConfigOptions. | ||
| pub fn Export( |
There was a problem hiding this comment.
This is the intended entrypoint for the exporter to call into the OTLP transport.
Signed-off-by: inge4pres <[email protected]>
inge4pres
force-pushed
the
metrics/stable-exporters-2
branch
from
903e852 to
68590a9
Compare
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
Signed-off-by: inge4pres <[email protected]>
inge4pres
force-pushed
the
metrics/stable-exporters-2
branch
from
68590a9 to
5f679a0
Compare
|
Rebased and fixed conflicts, please review 🙏🏼 |
There was a problem hiding this comment.
Hi @inge4pres , the changes looks fine to me. Hence, approving from my side...
Reason for this PR
Note
This is a big PR
Reviewing it commit by commit might be easier
Part of #15: adds OTLP protocol implementation and its exporter counterpart, consuming the protocol.
Details
We are restricting the implementation to the bare minimum that satisfies the specification:
protobuf-zig, but I may be wrong)MetricExporterdesign previously delivering the other stable exportersSome miscellaneous additions and fixes arose from getting the protobuf conversion running, they are included in this PR.
Added context
There is not great test coverage for the OTLP exporter, mainly because I am struggling to find a decent way of mocking the client.
The goal for this PR is to start filling in the gaps, implementing the code.
I will raise another issue wrt to testing OTLP, possibly with some integration tests or in the examples.
In the issue I'll mention that if we're able to spin up a Collector test harness (using the
opentelemetry/proto/collectorprotobuf definitions, testing might be easier.On top of that, the Collector messages code-generation will have to be done anyway to handle Partial success and Failure messages (which are now simply logged).