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

Add OpenTelemetry Meter API #162

Merged
merged 55 commits into from
Aug 3, 2020
Merged

Add OpenTelemetry Meter API #162

merged 55 commits into from
Aug 3, 2020

Conversation

Brandon-Kimberly
Copy link
Contributor

@Brandon-Kimberly Brandon-Kimberly commented Jul 8, 2020

I am working on implementing the metrics API and SDK for C++. This is one of a series of PRs I plan to file along with @ankit-bhargava and @HudsonHumphries.

This is a cut at implementing the API's Meter Class defined in the specification. This class provides functions to create and return all metric instrument types. It also provides a function to record to a batch of instruments in a single API call.

I have also included the default no-op implementation of the meter class in metrics/noop.h. The default no-op MeterProvider will instantiate this class. This class is only capable of creating and returning no-op metric instruments.

Lastly, I have included a no-op meter test. This is similar to the no-op tracer test and checks that there are no errors when creating metric instruments and calling BatchRecord.

Note: This PR does not currently build. The exact reason why is listed in the review section below.

@Brandon-Kimberly
Copy link
Contributor Author

Brandon-Kimberly commented Jul 25, 2020

I have updated the meter class to address some feedback left on this PR and to improve integration with other metrics classes. It is ready to re-review.

NOTE:
However, this PR is still not ready to merge as the RecordBatch test fails for a similar reason listed in PR #212 and expounded up in issue #216. The nostd::shared_ptr class does not implement std::static_pointer_cast and therefore the implicit upcasts from a counter instrument to SynchronousInstrument in the RecordBatch functions do not work correctly. Once nostd::shared_ptr can implicitly upcast then this test will pass and this PR should be ready to merge.

@Brandon-Kimberly Brandon-Kimberly changed the title Add OpenTelemetry Meter API [WIP] Add OpenTelemetry Meter API Jul 25, 2020
I could not get the counter upcasting to work correctly for some reason.
Replace overloaded RecordBatch functions.
@Brandon-Kimberly Brandon-Kimberly changed the title Add OpenTelemetry Meter API Add OpenTelemetry Meter API [WIP] Jul 27, 2020
Note that this is definitely not ideal for users. Making this function more user-friendly is a potential future enhancement to our code.
@Brandon-Kimberly Brandon-Kimberly changed the title Add OpenTelemetry Meter API [WIP] Add OpenTelemetry Meter API Jul 30, 2020
@Brandon-Kimberly
Copy link
Contributor Author

Hello all, I have updated the Meter class to remove the need to use dynamic_pointer_cast. However, this requires working with raw pointers in the RecordBatch function. This is not ideal but it is required due to limitations of nostd::shared_ptr.

This PR does not currently pass CI tests due to an issue in the MeterProvider class that was already merged. However, once that is fixed this PR will be ready to merge, pending final approval.

This is necessary because the MeterProvider class (that is already merged into the repo) instantiates this class and the Meter SDK stub in the repo will have unimplemented pure virtual functions if this PR is merged.

This stub will be updated with the actual header when the Meter SDK class is merged (pending approval in PR #212).
@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #162 into master will decrease coverage by 2.03%.
The diff coverage is 43.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #162      +/-   ##
==========================================
- Coverage   94.22%   92.19%   -2.04%     
==========================================
  Files         115      117       +2     
  Lines        3882     4038     +156     
==========================================
+ Hits         3658     3723      +65     
- Misses        224      315      +91     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/metrics/meter.h 6.66% <3.44%> (-93.34%) ⬇️
api/include/opentelemetry/metrics/noop.h 45.03% <47.54%> (-0.89%) ⬇️
api/test/metrics/noop_metrics_test.cc 88.63% <88.63%> (ø)
api/include/opentelemetry/metrics/meter.h 100.00% <100.00%> (ø)
sdk/src/metrics/meter_provider.cc 100.00% <0.00%> (ø)

@Brandon-Kimberly
Copy link
Contributor Author

@reyang This PR is ready to merge, pending final approval. I had to add a stubbed sdk/meter class so that the MeterProvider test would pass. This is only a stubbed version and will be overwritten when PR #212 is merged.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 30, 2020
@Brandon-Kimberly
Copy link
Contributor Author

@reyang I believe this PR is ready to merge :)

@reyang reyang merged commit 5298132 into open-telemetry:master Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants