[EXPORTER] Implement in-memory metric exporter by punya · Pull Request #3043 · open-telemetry/opentelemetry-cpp

punya · 2024-09-01T00:14:47Z

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2024-09-01T00:39:59Z

Codecov Report

Attention: Patch coverage is 96.38554% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.79%. Comparing base (497eaf4) to head (0dae157).
Report is 129 commits behind head on main.

Files with missing lines	Patch %	Lines
exporters/memory/src/in_memory_metric_data.cc	84.22%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3043      +/-   ##
==========================================
+ Coverage   87.12%   87.79%   +0.68%     
==========================================
  Files         200      195       -5     
  Lines        6109     5953     -156     
==========================================
- Hits         5322     5226      -96     
+ Misses        787      727      -60

Files with missing lines	Coverage Δ
...telemetry/exporters/memory/in_memory_metric_data.h	`100.00% <100.00%> (ø)`
...rs/memory/src/in_memory_metric_exporter_factory.cc	`100.00% <100.00%> (ø)`
...xporters/memory/test/in_memory_metric_data_test.cc	`100.00% <100.00%> (ø)`
...ters/memory/test/in_memory_metric_exporter_test.cc	`100.00% <100.00%> (ø)`
exporters/memory/src/in_memory_metric_data.cc	`84.22% <84.22%> (ø)`

... and 125 files with indirect coverage changes

marcalff

Thanks for the feature.

See some preliminary comments.

exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h

lalitb · 2024-09-04T07:07:47Z

Thanks for the PR. LGTM as first version. Couple of improvements I can think for future:

InMemoryMetricData could be better represented as std::map to easily get the actual metric data in case of multiple meters, each with multiple instruments and each instrument emitting measurements with different attributes. This would make it easy to integrate in unit-tests. Something like below:

//  Tuple: (name of the instrumentation scope, name of instrument descriptor)
//  e.g. ("library_name", "metrics_name")
using InMemoryMetricDataKey = std::tuple<std::string, std::string>;
using InMemoryMetricData = std::map<
    InMemoryMetricDataKey,
    std::map<opentelemetry::sdk::metrics::PointAttributes, opentelemetry::sdk::metrics::PointType>>;

InMemoryMetricExporter::Clear() method for cleanup the data, and to enable using the exporter instance for next collection test.

exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h

marcalff

Thanks for the feature.

Please see some changes, just moving code around in different files.

punya · 2024-09-05T02:46:15Z

Thanks for the comments and detailed explanations @marcalff - now I understand why the in-memory span exporter is designed the way it is.

marcalff

LGTM, thanks for the feature.

Please check the CI logs and search for "error:" in maintainer builds,
and resolve remaining build warnings.

marcalff · 2024-09-05T21:43:54Z

Also, please check the CI for include-what-you-use (iwyu), search the logs for in_memory_metric, and fix reported issues.

The iwyu build is not enforced yet, but the goal is to get to 0 issues and then enforce it.

For example:

[ 87%] Building CXX object exporters/memory/CMakeFiles/opentelemetry_exporter_in_memory_metric.dir/src/in_memory_metric_exporter_factory.cc.o
Warning: include-what-you-use reported diagnostics:

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter_factory.h should add these lines:
#include <memory>                                            // for shared_ptr

/home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter_factory.h should remove these lines:
- #include <atomic>  // lines 6-6
- #include <vector>  // lines 7-7
- #include "opentelemetry/exporters/memory/in_memory_data.h"  // lines 9-9
- #include "opentelemetry/sdk/metrics/export/metric_producer.h"  // lines 10-10
- namespace opentelemetry { namespace v1 { namespace exporter { namespace memory { class InMemoryMetricData; } } } }  // lines 21-21

The full include-list for /home/runner/work/opentelemetry-cpp/opentelemetry-cpp/exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter_factory.h:
#include <memory>                                            // for shared_ptr
#include "opentelemetry/sdk/metrics/instruments.h"           // for Aggregat...
#include "opentelemetry/sdk/metrics/push_metric_exporter.h"  // for PushMetr...
#include "opentelemetry/version.h"                           // for OPENTELE...
---

punya · 2024-09-06T19:31:05Z

Thanks for the feedback. I’ll debug the CI failures over the weekend.

punya · 2024-09-08T16:14:31Z

My apologies to the maintainers for the confusing Git history (i.e. repeated force pushes). I've been trying out a different source control workflow locally and made a few mistakes. I plan to do this in a more streamlined fashion for future reviews.

Fixes #1405

punya marked this pull request as ready for review September 1, 2024 03:55

punya requested a review from a team September 1, 2024 03:55

marcalff reviewed Sep 3, 2024

View reviewed changes