feat: add self-observability metrics to otlpmetricgrpc metric exporters#7120
feat: add self-observability metrics to otlpmetricgrpc metric exporters#7120minimAluminiumalism wants to merge 15 commits intoopen-telemetry:mainfrom
Conversation
minimAluminiumalism
requested review from
MrAlias,
XSAM,
dashpole,
dmathieu,
flc1125 and
pellared
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7120 +/- ##
======================================
Coverage 82.9% 82.9%
======================================
Files 264 266 +2
Lines 24628 24779 +151
======================================
+ Hits 20425 20561 +136
- Misses 3820 3832 +12
- Partials 383 386 +3
🚀 New features to boost your workflow:
|
|
@pellared I've tried to fix the CI pipeline failures, please review this PR and re-trigger the pipeline check. |
|
@minimAluminiumalism, can you please make the documentation and changelog entry similar to other modules? You can take a look at #7121 for another reference. |
The changelog and doc.go have been modified. |
|
Since this PR includes 2 components, I suggest implementing one of them first, as it will be easier to get through during both implementation and review. |
|
As a supplementary note (which might be useful), when conducting unit tests for metrics, we can use this package: https://github.com/open-telemetry/opentelemetry-go/tree/main/sdk/metric/metricdata/metricdatatest In fact, I haven't started the review of unit tests yet. (Reason: The functional part has not yet been fully reviewed). |
I've removed the implementation of otlpmetrichttp, please review it. |
flc1125
changed the title
flc1125
changed the title
flc1125
left a comment
flc1125
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
|
@flc1125 Conflicts resolved, re-run the pipeline please :) |
|
@flc1125 Maybe it's time to merge this PR? I'll implement it of |
Sorry, I was on vacation last week, and there have been some significant recent changes that I need to review. |
|
Please use this component https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters/stdout/stdouttrace as a reference; there have been some recent optimizations, and we need to adjust accordingly. You can view the change log via this link: https://github.com/open-telemetry/opentelemetry-go/commits/main/exporters/stdout/stdouttrace Please refer to the adjustment for this range: 2ab7d4d...c2a6172 |
|
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~ |
|
@minimAluminiumalism please take a look at #7012. I have updated the issue with a check list of items from our project Observability guidelines that need to be completed in this PR. As @flc1125 pointed out, you may find it easier to open a new PR. But feel free to update this PR as well. If you plan to update this PR, please mark it as a draft while you are working to address the items listed in #7012 and the Observability guidelines. |
Ok, I'll submit a new PR. |
|
@minimAluminiumalism just wanted to check in on this. Are you still able to update this PR or open another PR to address the instrumentation requirements? |
I just finished my vacation. And I'll finish this PR and submit it in the next few days. |
|
@minimAluminiumalism, any update on your side? Are you able to update the PR? |
|
@minimAluminiumalism, kindly reminder. |
| dataPointCount := countDataPoints(rm) | ||
| startTime := time.Now() | ||
|
|
||
| em.inflight.Add(ctx, dataPointCount, em.attrs...) |
There was a problem hiding this comment.
This and calls below should be pooling options. See https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#attribute-and-option-allocation-management
|
|
||
| duration := time.Since(startTime).Seconds() | ||
|
|
||
| attrs := make([]attribute.KeyValue, len(em.attrs), len(em.attrs)+1) |
There was a problem hiding this comment.
You should pool attribute slices here.
| "go.opentelemetry.io/otel/metric" | ||
| "go.opentelemetry.io/otel/sdk" | ||
| "go.opentelemetry.io/otel/sdk/metric/metricdata" | ||
| semconv "go.opentelemetry.io/otel/semconv/v1.36.0" |
| dataPointCount := countDataPoints(rm) | ||
| startTime := time.Now() | ||
|
|
||
| em.inflight.Add(ctx, dataPointCount, em.attrs...) |
There was a problem hiding this comment.
You should check if each instrument is Enabled(ctx) prior to making Add or Record calls.
|
I'll pick this PR up to get it over the finish line since it hasn't been updated in a few months. Thanks for all your hard work @minimAluminiumalism! |
5 participants
Fixes #7012
internal/selfobservabilitypackage to gRPC exportersotel.sdk.exporter.metric_data_point.inflight(UpDownCounter)otel.sdk.exporter.metric_data_point.exported(Counter)otel.sdk.exporter.operation.duration(Histogram)