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

[EXPORTER] Implement in-memory metric exporter #3043

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

punya
Copy link
Member

@punya punya commented Sep 1, 2024

Fixes #1405

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

@punya punya force-pushed the in-memory-metric-exporter branch 2 times, most recently from 989599c to 065f63c Compare September 1, 2024 00:28
Copy link

codecov bot commented Sep 1, 2024

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

Impacted file tree graph

@@            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

@punya punya force-pushed the in-memory-metric-exporter branch 9 times, most recently from 72ea75d to 1478eb6 Compare September 1, 2024 03:35
@punya punya marked this pull request as ready for review September 1, 2024 03:55
@punya punya requested a review from a team September 1, 2024 03:55
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for the feature.

See some preliminary comments.

@punya punya force-pushed the in-memory-metric-exporter branch 2 times, most recently from 3acf07c to 6f82c86 Compare September 3, 2024 21:22
@lalitb
Copy link
Member

lalitb commented Sep 4, 2024

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

  1. 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>>;
  1. InMemoryMetricExporter::Clear() method for cleanup the data, and to enable using the exporter instance for next collection test.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for the feature.

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

@marcalff marcalff changed the title Implement in-memory metric exporter [EXPORTER] Implement in-memory metric exporter Sep 4, 2024
@punya
Copy link
Member Author

punya commented Sep 5, 2024

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

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the feature.

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

@marcalff
Copy link
Member

marcalff commented Sep 5, 2024

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
Copy link
Member Author

punya commented Sep 6, 2024

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

@punya punya closed this Sep 8, 2024
@punya punya reopened this Sep 8, 2024
@punya

This comment was marked as resolved.

@punya punya marked this pull request as draft September 8, 2024 16:07
@punya
Copy link
Member Author

punya commented Sep 8, 2024

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.

@punya punya force-pushed the in-memory-metric-exporter branch 5 times, most recently from 81ac6cc to a58c024 Compare September 8, 2024 19:16
@punya punya marked this pull request as ready for review September 9, 2024 01:29
@lalitb lalitb merged commit c37df23 into open-telemetry:main Sep 10, 2024
52 checks passed
@punya punya deleted the in-memory-metric-exporter branch September 10, 2024 14:28
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.

[Metrics SDK] Add InMemory metric exporter
3 participants