[mdatagen] Generate wrappers for recording telemetry metrics #12182
Conversation
crobert-1
requested review from
bogdandrutu,
dmitryax and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12182 +/- ##
==========================================
+ Coverage 91.57% 91.59% +0.01%
==========================================
Files 483 483
Lines 26383 26443 +60
==========================================
+ Hits 24160 24220 +60
Misses 1762 1762
Partials 461 461 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func (builder *TelemetryBuilder) RecordProcessorBatchBatchSendSize(ctx context.Context, val int64, opts ...metric.RecordOption) { | ||
| builder.ProcessorBatchBatchSendSize.Record(ctx, val, opts...) | ||
| } | ||
|
|
||
| func (builder *TelemetryBuilder) RecordProcessorBatchBatchSendSizeBytes(ctx context.Context, val int64, opts ...metric.RecordOption) { | ||
| builder.ProcessorBatchBatchSendSizeBytes.Record(ctx, val, opts...) | ||
| } | ||
|
|
||
| func (builder *TelemetryBuilder) RecordProcessorBatchBatchSizeTriggerSend(ctx context.Context, val int64, opts ...metric.AddOption) { | ||
| builder.ProcessorBatchBatchSizeTriggerSend.Add(ctx, val, opts...) | ||
| } | ||
|
|
||
| func (builder *TelemetryBuilder) RecordProcessorBatchTimeoutTriggerSend(ctx context.Context, val int64, opts ...metric.AddOption) { | ||
| builder.ProcessorBatchTimeoutTriggerSend.Add(ctx, val, opts...) | ||
| } |
There was a problem hiding this comment.
Few things here:
- We will confuse the user when using Add/Record because we don't respect the same semantics as otel.
- What is the next level for this PR? Can you just provide a sketch code that you want to end with, please don't make the code changes, just provide a draft code for me to understand.
There was a problem hiding this comment.
- We will confuse the user when using Add/Record because we don't respect the same semantics as otel.
Good call out. My thought process was to make it as simple and consistent for the user as possible. This also matches mdatagen's functionality for metrics, which always have Record as the prefix irrespective of metric type.
- What is the next level for this PR? Can you just provide a sketch code that you want to end with, please don't make the code changes, just provide a draft code for me to understand.
The goal is to bring up internal telemetry functionality to the same level as receiver metrics using mdatagen. My intention would be to make the metric itself private to the internal/metadata package within a component, then only expose the Record method.
Something like:
type AttributeFoo string
type metricProcessorBatchBatchSendSize struct {
}
type TelemetryBuilder {
metricProcessorBatchBatchSendSendSize metricProcessorBatchBatchSendSendSize
}
func (builder *TelemetryBuilder) RecordProcessorBatchBatchSendSendSize(ctx context.Context, val int64, foo string) {
}
Going this direction allows us to implement support for the documented schema of mdatagen, especially telemetry metric attributes, and enforcing the set value for enabled in the schema.
I can open an issue with this proposal if it would be better for tracking and discussion.
There was a problem hiding this comment.
- @bogdandrutu, do you want to use
Add<MetricName>for sum type? That sounds reasonable. We don't haveDataPointsuffix here as we have in the normal builder, and that makes sense. So we can probably provide an interface closer to the go SDK instead.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
crobert-1
force-pushed
the
tel_metric_attributes
branch
from
20bc933 to
de5c4d1
Compare
crobert-1
force-pushed
the
tel_metric_attributes
branch
from
de5c4d1 to
29aefac
Compare
|
Apologies for letting this go stale, I've updated. @bogdandrutu I've replied to your question, can you review again when you get a chance? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
crobert-1
force-pushed
the
tel_metric_attributes
branch
from
230e064 to
9065f51
Compare
Description
This is essentially reviving #10911, the first step towards #10801. The goal is to be able to use
mdatagento handle more use cases around telemetry metrics, similar to regular metrics, especially to be able to set attributes on the telemetry metrics.Link to tracking issue
Part of #10801
Testing
Code has been generated for different telemetry metric types, existing tests use this behavior pretty heavily.