-
Notifications
You must be signed in to change notification settings - Fork 417
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
Conversation
989599c
to
065f63c
Compare
Codecov ReportAttention: Patch coverage is
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
|
72ea75d
to
1478eb6
Compare
1478eb6
to
21bb7b5
Compare
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.
Thanks for the feature.
See some preliminary comments.
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
3acf07c
to
6f82c86
Compare
Thanks for the PR. LGTM as first version. Couple of improvements I can think for future:
// 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>>;
|
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
exporters/memory/include/opentelemetry/exporters/memory/in_memory_metric_exporter.h
Outdated
Show resolved
Hide resolved
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.
Thanks for the feature.
Please see some changes, just moving code around in different files.
Thanks for the comments and detailed explanations @marcalff - now I understand why the in-memory span exporter is designed the way it is. |
6f82c86
to
48aa31a
Compare
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.
LGTM, thanks for the feature.
Please check the CI logs and search for "error:" in maintainer builds,
and resolve remaining build warnings.
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:
|
Thanks for the feedback. I’ll debug the CI failures over the weekend. |
3d51b3d
to
4eb78a6
Compare
This comment was marked as resolved.
This comment was marked as resolved.
a91e933
to
abc560b
Compare
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. |
81ac6cc
to
a58c024
Compare
a58c024
to
0dae157
Compare
Fixes #1405
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes