Skip to content
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

Improve efficiency of ETW metrics exporter #134

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

mattbodd
Copy link
Contributor

@mattbodd mattbodd commented Dec 3, 2024

Fixes #104
Design Decisions:

  • Follow .Net and C++ implementations and use helper methods to serialize fragments of metric points into a coherent, exportable buffer
  • Construct a new ResourceMetrics for each metric point and serialize each into an exportable buffer

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 75.42373% with 29 lines in your changes missing coverage. Please review.

Project coverage is 53.9%. Comparing base (72a6d4b) to head (47a941d).

Files with missing lines Patch % Lines
opentelemetry-etw-metrics/src/exporter/mod.rs 75.4% 29 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #134     +/-   ##
=======================================
- Coverage   54.1%   53.9%   -0.2%     
=======================================
  Files         42      42             
  Lines       6279    6149    -130     
=======================================
- Hits        3397    3316     -81     
+ Misses      2882    2833     -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mattbodd
Copy link
Contributor Author

Comparing the latest exporter benchmark results between the proposed export implementation and the previous export implementation, there is a ~26% improvement.

@mattbodd mattbodd marked this pull request as ready for review December 17, 2024 20:42
@mattbodd mattbodd requested a review from a team as a code owner December 17, 2024 20:42
@mattbodd
Copy link
Contributor Author

Comparing the latest exporter benchmark results between the proposed export implementation and the previous export implementation, there is a ~26% improvement.

With a more realistic ResourceMetrics, the improvement looks to be closer to 38%

@psandana
Copy link
Contributor

We noticed some tonic namespace usage along with types with Tonic prefix. Discussing with @cijothomas we got clarified OpenTelemetry is not using Tonic, they are just types coming from proto definitions.

Thus, this comment is not blocking as the change is unrelated to this PR, but we propose making those proto types more generic, thus we prevent future confusions regarding tonic usage or not.

…buffer in emit_export_metric_service_request
@mattbodd
Copy link
Contributor Author

We noticed some tonic namespace usage along with types with Tonic prefix. Discussing with @cijothomas we got clarified OpenTelemetry is not using Tonic, they are just types coming from proto definitions.

Thus, this comment is not blocking as the change is unrelated to this PR, but we propose making those proto types more generic, thus we prevent future confusions regarding tonic usage or not.

I borrowed the use pattern set in open-telemetry/opentelemetry-rust/opentelemetry-proto/src/transform/metrics.rs. @psandana or @cijothomas do you have suggestions for better generic names?

fn emit_export_metric_service_request(
export_metric_service_request: &ExportMetricsServiceRequest,
) -> MetricResult<()> {
let mut encoding_buffer = Vec::with_capacity(export_metric_service_request.encoded_len());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR but we should be able to reuse the vec instead of allocating a new one for each export. MetricExporter struct can hold a vec as one of its fields and keeping using that vec for each export.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@utpilla good call out. I'll leave that work to a future PR if that's ok with everyone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I make a tweak that's part of the way towards this improvement. Now emit_export_metric_service_request takes a &mut Vec<u8> as a parameter to avoid allocating a fresh Vec<u8> every invocation. Now a new vector is only initialized once per export.


for _i in 0..iters {
runtime.block_on(async {
export(&exporter, &mut resource_metrics).await;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to micro benchmark just the serialization and export of one data point instead of 20000. It would keep the calculations simple when comparing two benchmark results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is coming from - #134 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Etw-Metrics - Improvements required
6 participants