stdoutmetric: self-observability: exporter metrics #7150
Conversation
…GO_X_SELF_OBSERVABILITY
…etrics 1. otel.sdk.exporter.metric_data_point.inflight 2. otel.sdk.exporter.metric_data_point.exported 3. otel.sdk.exporter.operation.duration
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7150 +/- ##
======================================
Coverage 82.8% 82.8%
======================================
Files 265 268 +3
Lines 24896 25018 +122
======================================
+ Hits 20631 20737 +106
- Misses 3885 3897 +12
- Partials 380 384 +4
🚀 New features to boost your workflow:
|
mahendrabishnoi2
changed the title
mahendrabishnoi2
requested review from
MrAlias,
XSAM,
dashpole,
dmathieu,
flc1125 and
pellared
as code owners
mahendrabishnoi2
left a comment
mahendrabishnoi2
left a comment
There was a problem hiding this comment.
I don't like how verbose the tests are. Looking for feedback on tests.
| if err != nil { | ||
| attrs = make([]attribute.KeyValue, len(em.attrs)+1) | ||
| copy(attrs, em.attrs) | ||
| attrs = append(attrs, semconv.ErrorType(err)) |
There was a problem hiding this comment.
semconv.ErrorType - I have copied this from another PR that is not merged yet.
Reason: for stdout metric exporter, I'm not able to think of a set of errors that can be defined and used like other exporters (queue_full etc.)
Looking for feedback.
| otel.Handle(err) | ||
| } | ||
|
|
||
| em.attrs = []attribute.KeyValue{componentName, componentType} |
There was a problem hiding this comment.
As per specification, it requires host and port attributes as well. Not sure what the correct values would be for stdoutmetric exporter.
Looking for feedback.
exporters/stdout/stdoutmetric/internal/selfobservability/selfobservability.go
Outdated
Show resolved
Hide resolved
flc1125
left a comment
flc1125
left a comment
There was a problem hiding this comment.
reviewed: functional parts only (excluding unit tests)
exporters/stdout/stdoutmetric/internal/selfobservability/selfobservability.go
Outdated
Show resolved
Hide resolved
exporters/stdout/stdoutmetric/internal/selfobservability/selfobservability.go
Outdated
Show resolved
Hide resolved
mahendrabishnoi2
force-pushed
the
stdoutmetric-auto-instrumentation
branch
from
ff285e6 to
460f303
Compare
exporters/stdout/stdoutmetric/internal/selfobservability/selfobservability.go
Outdated
Show resolved
Hide resolved
| rm := &metricdata.ResourceMetrics{ | ||
| ScopeMetrics: []metricdata.ScopeMetrics{ | ||
| { | ||
| Metrics: []metricdata.Metrics{ | ||
| { | ||
| Name: "gauge_int64", | ||
| Data: metricdata.Gauge[int64]{ | ||
| DataPoints: []metricdata.DataPoint[int64]{{Value: 1}, {Value: 2}}, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "gauge_float64", | ||
| Data: metricdata.Gauge[float64]{ | ||
| DataPoints: []metricdata.DataPoint[float64]{ | ||
| {Value: 1.0}, | ||
| {Value: 2.0}, | ||
| {Value: 3.0}, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "sum_int64", | ||
| Data: metricdata.Sum[int64]{ | ||
| DataPoints: []metricdata.DataPoint[int64]{{Value: 10}}, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "sum_float64", | ||
| Data: metricdata.Sum[float64]{ | ||
| DataPoints: []metricdata.DataPoint[float64]{{Value: 10.5}, {Value: 20.5}}, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "histogram_int64", | ||
| Data: metricdata.Histogram[int64]{ | ||
| DataPoints: []metricdata.HistogramDataPoint[int64]{ | ||
| {Count: 1}, | ||
| {Count: 2}, | ||
| {Count: 3}, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "histogram_float64", | ||
| Data: metricdata.Histogram[float64]{ | ||
| DataPoints: []metricdata.HistogramDataPoint[float64]{{Count: 1}}, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "exponential_histogram_int64", | ||
| Data: metricdata.ExponentialHistogram[int64]{ | ||
| DataPoints: []metricdata.ExponentialHistogramDataPoint[int64]{ | ||
| {Count: 1}, | ||
| {Count: 2}, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "exponential_histogram_float64", | ||
| Data: metricdata.ExponentialHistogram[float64]{ | ||
| DataPoints: []metricdata.ExponentialHistogramDataPoint[float64]{ | ||
| {Count: 1}, | ||
| {Count: 2}, | ||
| {Count: 3}, | ||
| {Count: 4}, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Name: "summary", | ||
| Data: metricdata.Summary{ | ||
| DataPoints: []metricdata.SummaryDataPoint{{Count: 1}}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| err = exp.Export(ctx, rm) | ||
| require.NoError(t, err) | ||
|
|
||
| var metrics metricdata.ResourceMetrics | ||
| err = reader.Collect(ctx, &metrics) | ||
| require.NoError(t, err) | ||
|
|
||
| var foundExported, foundDuration, foundInflight bool | ||
| var exportedCount int64 | ||
|
|
||
| for _, sm := range metrics.ScopeMetrics { | ||
| for _, m := range sm.Metrics { | ||
| switch m.Name { | ||
| case otelconv.SDKExporterMetricDataPointExported{}.Name(): | ||
| foundExported = true | ||
| if sum, ok := m.Data.(metricdata.Sum[int64]); ok { | ||
| for _, dp := range sum.DataPoints { | ||
| exportedCount += dp.Value | ||
| } | ||
| } | ||
| case otelconv.SDKExporterOperationDuration{}.Name(): | ||
| foundDuration = true | ||
| case otelconv.SDKExporterMetricDataPointInflight{}.Name(): | ||
| foundInflight = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| assert.Equal(t, tt.selfObservabilityEnabled, foundExported) | ||
| assert.Equal(t, tt.selfObservabilityEnabled, foundDuration) | ||
| assert.Equal(t, tt.selfObservabilityEnabled, foundInflight) | ||
| assert.Equal(t, tt.expectedExportedCount, exportedCount) |
There was a problem hiding this comment.
Maybe it’s just my OCD, but could we reorganize the unit tests here?
Also, we can use
for testing.There was a problem hiding this comment.
Good suggestion. I have merged both tests cases and using metricdatatest.AssertEqual now.
Could you please check this again and see if anything else can be improved?
exporters/stdout/stdoutmetric/internal/selfobservability/selfobservability_test.go
Show resolved
Hide resolved
- use pool to amortize slice allocation - pass actual context - use t.Cleanup instead of defer in tests - improve readability by returning without using err var
- use metricdatatest for comparision in testcase
|
@mahendrabishnoi2 PTAL #7272 |
|
Hi, since this process involves specification adjustments and historical review records (which may contain invalid review suggestions), it’s impossible to tell which items need attention amid the large volume of information. Could we create a new PR based on the current branch before preparing for the review, so that subsequent reviews can proceed more smoothly? Thanks~ |
|
@mahendrabishnoi2 similar to #7162 (comment), can you take a look at #7014 for the checklist that needs to be completed from this issue? Also, feel free to close or mark this as a draft while you work through the next steps. |
|
@mahendrabishnoi2 Hi, before the new PR arrives, I would like to set this PR to draft mode. |
Sure @flc1125. I didn't get option to do so. |
It has been switched to draft mode. |
|
@mahendrabishnoi2 I just wanted to check in with this PR. Are you still able to update this PR or open a new PR to address the instrumentation requirements? |
Yes @MrAlias. I'm planning to close #7374 first. I was hoping to get inputs on #7374 (comment) and #7374 (comment). |
|
Superseded by #7492 |
5 participants
Fixes #7014
This PR adds support for below self observability metrics for stdoutmetric exporter
otel.sdk.exporter.metric_data_point.inflightotel.sdk.exporter.metric_data_point.exportedotel.sdk.exporter.operation.durationThese metrics are experimental and hence behind a feature flag
OTEL_GO_X_SELF_OBSERVABILITY.Definition of above metrics is available at https://github.com/open-telemetry/semantic-conventions/blob/v1.36.0/docs/otel/sdk-metrics.md