Add the in-memory metrics exporter.

[yxue]

trafficstars

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:

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

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: yxue / name: Yan Xue (08cdb56dd14461be6038f13c4873a72b8558da59)

[codecov[bot]]

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%)	:arrow_up:

[owent]

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

[yxue]

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]

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 https://github.com/open-telemetry/opentelemetry-cpp/pull/666 , do you has any suggestion about it?

[lalitb]

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]

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]

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]

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]

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., https://github.com/open-telemetry/opentelemetry-cpp/issues/1428. I think we might need some tools to reconcile that if the community is going to keep two building systems.

[lalitb]

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]

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!

[owent]

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

[yxue]

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]

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.

[lalitb]

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]

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!

[ThomsonTan]

@yxue could you please resolve the merge conflicts?