-
Notifications
You must be signed in to change notification settings - Fork 848
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
introduction of View and first version of LabelsProessor #2800
introduction of View and first version of LabelsProessor #2800
Conversation
as-polyakov
commented
Feb 11, 2021
- Added View as a wrapper around AggregatorFactory and LabelsProcessorFactory
- Added LabelsProcessor interfaces
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProvider.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/processor/LabelsProcessor.java
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.
Initial feedback about the new interfaces
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/processor/BaggageLabelsProcessor.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/processor/LabelsProcessor.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/processor/LabelsProcessor.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/processor/LabelsProcessorFactory.java
Outdated
Show resolved
Hide resolved
import io.opentelemetry.api.metrics.common.Labels; | ||
import io.opentelemetry.context.Context; | ||
|
||
public class NoopLabelsProcessor implements LabelsProcessor { |
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.
what about we make create
return "nullable" value, and return null by default? That way we know if no processor is installed and we can optimize more in the accumulator (not even retrieving the current context, etc).
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.
I am not so sure about this one. Having nulls in public methods is usually considered an anti pattern, I do understand though that we have this call on a critical path for every metric recording. Seems like getting current context is pretty lightweight - just pulling it from thread local most of the time. Unless you insist I would avoid null's. If you do insist I am happy to change though
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.
you are correct, probably we need to expose just the noop instance, so at least we can do the trick if we want to compare with noop reference which should be same as comparing with null.
Codecov Report
@@ Coverage Diff @@
## main #2800 +/- ##
============================================
+ Coverage 89.50% 89.64% +0.13%
- Complexity 2613 2637 +24
============================================
Files 308 312 +4
Lines 8446 8481 +35
Branches 855 856 +1
============================================
+ Hits 7560 7603 +43
+ Misses 616 614 -2
+ Partials 270 264 -6
Continue to review full report at Codecov.
|
do we still need this PR now that the others have been merged? |
no
…On Fri, Mar 5, 2021 at 9:40 AM John Watson ***@***.***> wrote:
do we still need this PR now that the others have been merged?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2800 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC7E772ZMYN2AUZ6KRH7MHDTCEJQ3ANCNFSM4XODGJZQ>
.
|