-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Metrics SDK: Filtering metrics attributes #1191
Conversation
…pp into filter-attributes
Codecov Report
@@ Coverage Diff @@
## main #1191 +/- ##
==========================================
+ Coverage 93.29% 93.35% +0.06%
==========================================
Files 174 177 +3
Lines 6404 6502 +98
==========================================
+ Hits 5974 6069 +95
- Misses 430 433 +3
|
@@ -11,13 +11,25 @@ namespace metrics | |||
{ | |||
using MetricAttributes = opentelemetry::sdk::common::AttributeMap; | |||
|
|||
/** | |||
* The AttributesProcessor is responsible for customizing which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this can be made internal/private to the SDK for now.
The specification only allows very limited manipulation on attributes (e.g. remove certain attributes from the measurement), this generic solution could work but it comes with perf cost (e.g. the virtual AttributesProcessor::process
call).
Doesn't have to be a blocker though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do suspect some perf cost associated with virtual and attributes copy. I have added a benchmark test to get cost statistics and would be helpful if have to consider performance improvement.
Also, I realized that we need to calculate the hash on the attribute map to store metrics data. Have added functionality to calculate the hash based on std::hash
and boost::hash_combine`. As hash should be the same irrespective of the order of key/values in the AttributeMap, I have changed the internal storage for AttributeMap from unordered_map to map. This way we can avoid sorting of keys for every measurement.
Let me see how can we make this class internal/private
to SDK, as C++ doesn't have a direct way of doing it. We can make AttributeProcessor::process()
as private, and other SDK classes as its friend to it but would like to see a better approach if possible.
Doesn't have to be in this PR, we need to understand the performance characteristics (e.g. heap allocation, memory fragmentation, contention if there is any, etc.) in order to guide the SDK design/implementation. Here are some general guidelines regarding metrics memory usage https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/supplementary-guidelines.md#memory-management. Consider two types of performance tests:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to unblock this so we can get a working prototype end-to-end.
Thanks, this is useful. We will discuss these performance attributes in our community meeting and will come up with plan to measure them in a better way. |
…pp into filter-attributes
Fixes
#1190
Changes
As per the specs, Views should be able to configure the filtering of attributes reported on metrics, including removing all the attributes. This PR adds a filter attribute-processor for this. The list of attributes to be included in the metrics can be specified through this filter.
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes