Add the in-memory metrics exporter. by yxue · Pull Request #1481 · open-telemetry/opentelemetry-cpp

yxue · 2022-07-09T00:52:26Z

Fixes # (issue) 1405

Changes

Please provide a brief description of the changes here.

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

linux-foundation-easycla · 2022-07-09T00:52:29Z

The committers listed above are authorized under a signed CLA.

✅ login: yxue / name: Yan Xue (08cdb56)

codecov · 2022-07-09T05:26:06Z

Codecov Report

Merging #1481 (9040fec) into main (57bf8c2) will increase coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
+ Coverage   85.73%   85.79%   +0.06%     
==========================================
  Files         171      171              
  Lines        5240     5240              
==========================================
+ Hits         4492     4495       +3     
+ Misses        748      745       -3

Impacted Files	Coverage Δ
ext/src/http/client/curl/http_client_curl.cc	`81.44% <0.00%> (+1.14%)`	⬆️

owent · 2022-07-11T09:22:46Z

Could you please alse add cmake targets in exporters/memory/CMakeLists.txt and cmake/opentelemetry-cpp-config.cmake.in .

yxue · 2022-07-11T15:15:43Z

Could you please alse add cmake targets in exporters/memory/CMakeLists.txt and cmake/opentelemetry-cpp-config.cmake.in .

Sure, I can add that. I feel supporting/maintaining two build systems is a little bit inconvenient. Do we consider using some tools like, https://github.com/google/bazel-to-cmake, so that CMakeLists.txt can be generated from the BUILD and WORKSPACE, or using cmake and bazel (https://github.com/bazelbuild/rules_foreign_cc) supports importing cmake projects.

owent · 2022-07-11T17:26:06Z

Could you please alse add cmake targets in exporters/memory/CMakeLists.txt and cmake/opentelemetry-cpp-config.cmake.in .

Sure, I can add that. I feel supporting/maintaining two build systems is a little bit inconvenient. Do we consider using some tools like, https://github.com/google/bazel-to-cmake, so that CMakeLists.txt can be generated from the BUILD and WORKSPACE, or using cmake and bazel (https://github.com/bazelbuild/rules_foreign_cc) supports importing cmake projects.

Good point, I think it require more discussion. In most case, I think cmake users do not want to install another bazel toolset, and bazel users do not want install cmake toolset either.
cmake has many modules and tools to work with legacy build toolchains(pkg-config, makefile, autoconf, android ndk, feature dection by test running or compiling some c/c++ codes, environments and etc.).I'm not family with bazel and I'm not sure if it's easy to let bazel and cmake work together with these modules.

BTW: We had a discussion before and try to find a way to use bazel to build both with legacy gcc 4.8 and the latest compiler here #666 , do you has any suggestion about it?

lalitb · 2022-07-11T17:30:55Z

I feel supporting/maintaining two build systems is a little bit inconvenient

Yes, I do agree it's inconvenient to maintain both these build systems. The decision to support both was made much before any of the existing contributors joined the team. The SIG work was initiated by the folks from Google and Microsoft, and it was defacto choice to support the build systems used by these companies :). Having said that, most of the existing C++ projects use either(or both) of these build systems, and so it makes sense to keep supporting both.

ThomsonTan · 2022-07-11T17:46:12Z

The 2 build systems issue were discussed in the past as @lalitb and @owent mentioned. As there are users for both of them, we will try our best to keep both up to date and consistent. For the https://github.com/google/bazel-to-cmake, it has not been updated for a few years, not sure it could work, together with that we've already have full set of cmake files which is not expected to be overwriten.

lalitb · 2022-07-11T18:15:07Z

Thanks for the PR @yxue. This is your first PR here, and it is nicely done :)

Just thinking whether the InMemoryExporter should have any dependency on the protobuf. For Logs and Traces, the memory/ostream exporters are not dependent on any external library. Is it possible to achieve this for Metrics too - Eg by letting CircularBuffer contain the list of MetricData or ResourceMetrics ?

We can discuss further if it is something not feasible/or difficult to achieve with the current SDK implementation.

yxue · 2022-07-11T20:22:00Z

Thanks for the PR @yxue. This is your first PR here, and it is nicely done :)

Just thinking whether the InMemoryExporter should have any dependency on the protobuf. For Logs and Traces, the memory/ostream exporters are not dependent on any external library. Is it possible to achieve this for Metrics too - Eg by letting CircularBuffer contain the list of MetricData or ResourceMetrics ?

We can discuss further if it is something not feasible/or difficult to achieve with the current SDK implementation.

Thanks @lalitb! I was thinking about using the CircularBuffer with the ResourceMetrics but then we need the special process for the pointers inside the structure.

Having the MetricData with CircularBuffer might work but some info is lost.

Another thing is about comparing these structure objects. With protobuf message, the objects can be easily compared. Since we don't define the == for MetricsData and ResourceMetrics, comparing them in the unit test is not trivial.

yxue · 2022-07-11T20:40:17Z

Thanks @lalitb @owent and @ThomsonTan for the info about the building system.

I am not sure about fixing the gcc 4.8 and grpc compatibility. I do see the building systems are right now used by multiple teams/companies. I also saw there are already some divergence problems between these two systems, e.g., #1428. I think we might need some tools to reconcile that if the community is going to keep two building systems.

lalitb · 2022-07-12T02:04:51Z

Thanks @lalitb! I was thinking about using the CircularBuffer with the ResourceMetrics but then we need the special process for the pointers inside the structure

Yes, I thought so too. Should be fine to keep the dependency unless someone has any concern :)
btw, I didn't realize this PR also has changes for bazel build for metrics proto files and raised a PR for that in #1489. Sorry about that, as you would have to resolve the conflict here :( Would ensure not to merge any further conflicting PRs till this is merged.

yxue · 2022-07-12T05:07:59Z

Thanks @lalitb! I was thinking about using the CircularBuffer with the ResourceMetrics but then we need the special process for the pointers inside the structure

Yes, I thought so too. Should be fine to keep the dependency unless someone has any concern :) btw, I didn't realize this PR also has changes for bazel build for metrics proto files and raised a PR for that in #1489. Sorry about that, as you would have to resolve the conflict here :( Would ensure not to merge any further conflicting PRs till this is merged.

Sure, no worries. The PR was updated. PTAL, thanks!

lalitb

LGTM.

owent · 2022-07-13T05:03:24Z

Could you please revert changes in /exporters/otlp ?It's duplicated with #1487 , I belive this PR is just for memory metrics exporter.

yxue · 2022-07-13T05:08:12Z

Could you please revert changes in /exporters/otlp ?It's duplicated with #1487 , I belive this PR is just for memory metrics exporter.

The change under exporters/otlp is for fixing the missing part of conversion between protobuf and structure. Remove that will fail the unit test.

owent · 2022-07-13T05:22:00Z

Could you please revert changes in /exporters/otlp ?It's duplicated with #1487 , I belive this PR is just for memory metrics exporter.

The change under exporters/otlp is for fixing the missing part of conversion between protobuf and structure. Remove that will fail the unit test.

OK. we can solve this conflict later.

owent

LGTM after in_memory_metric_exporter is also added into cmake/opentelemetry-cpp-config.cmake.in.

lalitb · 2022-07-13T18:35:54Z

LGTM after in_memory_metric_exporter is also added into cmake/opentelemetry-cpp-config.cmake.in.

Good point. @yxue this file contains the rules to enable opentelemetry-cpp included in other projects cmake files using find_package(opentelemetry-cpp). All the targets/libraries created by opentelemetry-cpp should be included in this file.

yxue · 2022-07-13T19:15:46Z

LGTM after in_memory_metric_exporter is also added into cmake/opentelemetry-cpp-config.cmake.in.

Good point. @yxue this file contains the rules to enable opentelemetry-cpp included in other projects cmake files using find_package(opentelemetry-cpp). All the targets/libraries created by opentelemetry-cpp should be included in this file.

Updated, thanks!

owent · 2022-07-14T02:53:50Z

+    opentelemetry_exporter_in_memory_metric
+    PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
+           "$<INSTALL_INTERFACE:include>"
+           "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/../otlp/include>")


target_link_libraries(opentelemetry_exporter_in_memory_metric PUBLIC opentelemetry_otlp_recordable) would be a better alternative here.But when we building without WITH_OTLP, opentelemetry_otlp_recordable will be unavailable.
Maybe this module should also be disabled when we build without WITH_OTLP now?

marcalff

LGTM

marcalff · 2022-07-17T17:27:19Z

+  /**
+   * @return Returns a shared pointer to this exporters InMemoryMetricData.
+   */
+  inline InMemoryMetricData &GetData() noexcept { return *data_; }


The comment is outdated, this does not return a shared pointer.

ThomsonTan · 2022-09-13T16:36:51Z

@yxue could you please resolve the merge conflicts?

marcalff

As discussed in C++ SIG meeting,
the in memory storage structure should not depend on protobuf:

  using InMemoryMetricData =
      opentelemetry::exporter::memory::InMemoryData<proto::metrics::v1::ResourceMetrics>;

Some refactoring needed to address this part.

yxue requested a review from a team July 9, 2022 00:52

yxue force-pushed the main branch from c202113 to b7204b7 Compare July 9, 2022 19:45

lalitb mentioned this pull request Jul 11, 2022

[Metrics] Otlp metric gRPC exporter has a wrong name. #1484

Closed

owent mentioned this pull request Jul 11, 2022

[Metrics] Add OTLP http metric exporter #1487

Merged

10 tasks

lalitb approved these changes Jul 13, 2022

View reviewed changes

Comment thread exporters/memory/src/in_memory_metric_exporter.cc Outdated

Comment thread exporters/memory/include/opentelemetry/exporters/memory/in_memory_data.h

Comment thread exporters/memory/test/in_memory_metric_exporter_test.cc

yxue mentioned this pull request Jul 13, 2022

Replace the in-memory span data with the templated in-memory data class #1493

Closed

owent requested changes Jul 13, 2022

View reviewed changes

yxue requested a review from owent July 13, 2022 20:20

owent approved these changes Jul 14, 2022

View reviewed changes

owent reviewed Jul 14, 2022

View reviewed changes

marcalff reviewed Jul 17, 2022

View reviewed changes

marcalff mentioned this pull request Aug 2, 2022

REQUEST: New membership for @marcalff open-telemetry/community#1122

Closed

6 tasks

lalitb self-assigned this Sep 26, 2022

marcalff requested changes Sep 28, 2022

View reviewed changes

github-actions Bot added the Stale label Nov 28, 2022

yxue force-pushed the main branch 16 times, most recently from 617b90b to 56ae726 Compare December 5, 2022 06:15

feat: Add memory metrics exporter.

9040fec

yxue force-pushed the main branch from 56ae726 to 9040fec Compare December 5, 2022 06:28

lalitb added do-not-stale and removed Stale labels Dec 5, 2022

lalitb closed this Jan 11, 2023

Conversation

yxue commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

linux-foundation-easycla Bot commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

owent commented Jul 11, 2022

Uh oh!

yxue commented Jul 11, 2022

Uh oh!

owent commented Jul 11, 2022

Uh oh!

lalitb commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ThomsonTan commented Jul 11, 2022

Uh oh!

lalitb commented Jul 11, 2022

Uh oh!

yxue commented Jul 11, 2022

Uh oh!

yxue commented Jul 11, 2022

Uh oh!

lalitb commented Jul 12, 2022

Uh oh!

yxue commented Jul 12, 2022

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owent commented Jul 13, 2022

Uh oh!

yxue commented Jul 13, 2022

Uh oh!

owent commented Jul 13, 2022

Uh oh!

owent left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalitb commented Jul 13, 2022

Uh oh!

yxue commented Jul 13, 2022

Uh oh!

owent Jul 14, 2022

Choose a reason for hiding this comment

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

ThomsonTan commented Sep 13, 2022

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yxue commented Jul 9, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Jul 9, 2022 •

edited

Loading

codecov Bot commented Jul 9, 2022 •

edited

Loading

lalitb commented Jul 11, 2022 •

edited

Loading

owent left a comment •

edited

Loading