[Instrumentation.AWS] Add AWS metrics instrumentation#1980
[Instrumentation.AWS] Add AWS metrics instrumentation#1980cijothomas merged 9 commits intoopen-telemetry:mainfrom
Conversation
github-actions
bot
added
the
comp:instrumentation.aws
|
Getting metrics added for the AWS SDK looks really cool in the Aspire Dashboard.
|
|
@muhammad-othman - Could you please take a look at failing tests? |
|
|
||
| namespace OpenTelemetry.Instrumentation.AWS.Implementation.Metrics; | ||
|
|
||
| internal class AWSHistogram<T> : Histogram<T> |
There was a problem hiding this comment.
what is the need of these wrappers over existing Histogram and others? Metrics API are designed to support high performance, allocation-free scenarios, and this wrappers is negating some of the perf optimizations... Wondering if this is really needed?
There was a problem hiding this comment.
We needed these wrappers to allow the AWS SDK to call these methods directly, and unfortunately the AWS SDK cannot take on new dependencies, and we supported .NET Standard and .NET Core 3.1, which lack System.Diagnostics.DiagnosticSource that is available in newer versions.
| { | ||
| if (attributes != null) | ||
| { | ||
| this.histogram.Record(value, attributes.AllAttributes.ToArray()); |
There was a problem hiding this comment.
The array allocation of heap is not desirable.
There was a problem hiding this comment.
unfortunately I couldn't avoid that with the current APIs, Histogram.Record accepts params as an array, and the API that the AWS SDK exposes uses IEnumerable since the attributes can be mutable in other places, maybe at some point Histogram.Record may accepts params as an IEnumerable which would be more flexible.
There was a problem hiding this comment.
Histogram.Record has many overloads to accepts attributes(Tags) without forcing allocation.
https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/metrics#metrics-api
It is unfortunate that the AWS wrapper is forcing heap allocation for metrics in hot path.
There was a problem hiding this comment.
@muhammad-othman When we move the SDK's observability changes into the V4 development branch we can add an AttributesAsSpan property and use it here to remove the allocation.
There was a problem hiding this comment.
Will create a v4 task for this, also this isn't the only place that we use .ToArray() so it should be useful to use it in other places including the tracing changes.
There was a problem hiding this comment.
can you add a TODO comment here that future aws sdk will offer api's to avoid allocation.?
… other tests running at the same time
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Glad to see more metrics instrumentation.
I am concerned about the heap allocated in the hot path, see #1980 (comment)
but given the owners have approved, will proceed to merge.
|
@srprash Could you review? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1980 +/- ##
===========================================
+ Coverage 73.91% 90.45% +16.54%
===========================================
Files 267 18 -249
Lines 9615 241 -9374
===========================================
- Hits 7107 218 -6889
+ Misses 2508 23 -2485
Flags with carried forward coverage won't be shown. Click here to find out more.
|
muhammad-othman
force-pushed
the
aws-metrics-instrumentation
branch
from
094b661 to
16fb984
Compare
6 participants
Changes
Integration of New Metrics APIs:
Implemented the new AWS SDK tracing APIs, starting with
AWSMeterProvider.Added Instrumentation Method:
Introduced
MeterProviderBuilderExtensions.AddAWSInstrumentationto enable metrics instrumentation.SDK Version Update:
Updated the SDK version to
3.7.400to incorporate the new tracing APIs.This update conflicts with the changes in PR #1974. The last merged PR will rebase accordingly.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes