[Metric SDK] - Avoid exposing AttributeSet to exporters - Part1 #1792
Conversation
cijothomas
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1792 +/- ##
=======================================
- Coverage 74.1% 73.0% -1.1%
=======================================
Files 125 121 -4
Lines 19481 18891 -590
=======================================
- Hits 14452 13808 -644
- Misses 5029 5083 +54 ☔ View full report in Codecov by Sentry. |
| attributes: attrs | ||
| .iter() | ||
| .map(|(k, v)| KeyValue::new(k.clone(), v.clone())) | ||
| .collect(), |
There was a problem hiding this comment.
not worried about the extra cost here, as this is in collect() path only, to focus on fixing the hot path first. The collect() path can be refactored afterwards.
There was a problem hiding this comment.
We can implement From trait, and use it throughout? And also, no need of cloning as we are already draining.
| @@ -97,7 +97,7 @@ impl<T: fmt::Debug + Send + Sync + 'static> Aggregation for Sum<T> { | |||
| pub struct DataPoint<T> { | |||
| /// Attributes is the set of key value pairs that uniquely identify the | |||
| /// time series. | |||
| pub attributes: AttributeSet, | |||
| pub attributes: Vec<KeyValue>, | |||
There was a problem hiding this comment.
Could we use a boxed slice instead of a Vec? It would save 8 bytes (usize length) per instance since unlike Vec we wouldn't need an additional pointer to track the capacity. That could save a considerable amount of memory if we are going to have a lot of these DataPoints.
| pub attributes: Vec<KeyValue>, | |
| pub attributes: Box<[KeyValue]>, |
There was a problem hiding this comment.
As mentioned in the PR desc, will revisit the public APIs. This PR is to unblock the unwanted exposure of AttributeSet outside of core sdk.
What other option we have ? From desc:
|
|
Will follow up offline on how best to handle Prometheus part with @TommyCpp |
IMO there is a difference between keeping it experimental(which means there will be breaking changes and bugs) and drops support completely. By removing it from CI it seems we are dropping support completely. By which point maybe it's better to move it to contrib repo |
|
We absolutely need it in main repo, as it is a spec-ed out component. (though experimental). Happy to discuss other ideas! Maybe good topic for the community meeting tommorow. |
AttributeSetis how attributes are stored internally for Metric Aggregation, and is also exposed to exporters. This PR exposes only aVec<KeyValue>to exporters (done only for SUM, HISTOGRAM to keep PR's small)In the next set of PRs, this will be extended to all Aggregations, and then
AttributeSetcan be completely kept pub(crate) instead of exposing it. Also, attributes can be further hidden by just exposing an Iterator only, instead of even a Vec!.This affects some export authors. Not adding changelog in this PR, as I plan to get it covered in one shot when above are achieved.
Also removed
opentelemetry-prometheusfrom CI for now, as it is not planned for the upcoming release : #1719