-
Notifications
You must be signed in to change notification settings - Fork 449
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
[SDK] Implement spec: MetricFilter #3235
base: main
Are you sure you want to change the base?
Conversation
|
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
bbd4ca5
to
9e8e4b7
Compare
d6f5504
to
1473ebe
Compare
Thanks for the PR. Please remember to sign the EasyCLA, so this PR can be processed. |
e0d8498
to
9c3f930
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3235 +/- ##
==========================================
+ Coverage 87.80% 87.87% +0.08%
==========================================
Files 198 199 +1
Lines 6324 6362 +38
==========================================
+ Hits 5552 5590 +38
Misses 772 772
|
a36ed14
to
a4384fa
Compare
@IcySteam Thanks for the PR. I believe you are working to get the CLA signed, as the PR can't be reviewed without that. |
a4384fa
to
4bff481
Compare
4bff481
to
7b19eb4
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 contribution, and for signing the CLA.
Before looking in more details at the code,
please see some preliminary comments below.
There should be no changes in third_party/opentelemetry-proto.
Please make sure to initialize and update git submodules in your branch.
Please check the "include-what-you-use" build in CI for warnings,
and fix the code to resolve them.
There should be no more warnings in the PR compared to the main branch.
More comments to follow.
void AddMetricReader(std::shared_ptr<MetricReader> reader) noexcept; | ||
std::shared_ptr<MetricCollector> AddMetricReader( | ||
std::shared_ptr<MetricReader> reader, | ||
std::unique_ptr<MetricFilter> metric_filter = nullptr) noexcept; |
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.
This was righty pointed out by @marcalff in community meeting - does this method need to return shared_ptr?
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.
@IcySteam It looks like you've now changed it to return a weak_ptr
. Sorry I was not clear earlier, the question was—why does this method need to return anything at all?
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.
@lalitb Returning the MetricCollector
created by the MeterContext
does seem like one of the only few ways to access the MetricCollector
, as it is needed by MetricCollectorTest
. Extending MetricCollector
with a test class is also potentially viable, but that would require changing some of MetricCollector
's private members to protected (or a friend class). What are your thoughts on writing proper tests in this case?
7b19eb4
to
72cbd97
Compare
72cbd97
to
de7b80e
Compare
Fixes #2447
Changes
This PR implements the MetricFilter spec in
opentelemetry-cpp
.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changesMetricProducer::MetricProducer
MetricCollector::MetricCollector
MeterContext::AddMetricReader
MeterProvider::AddMetricReader