-
Notifications
You must be signed in to change notification settings - Fork 843
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
Very basic Aggregation-configuration API in the SDK. #2037
Very basic Aggregation-configuration API in the SDK. #2037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2037 +/- ##
=========================================
Coverage ? 85.29%
Complexity ? 2070
=========================================
Files ? 235
Lines ? 8013
Branches ? 869
=========================================
Hits ? 6835
Misses ? 852
Partials ? 326
Continue to review full report at Codecov.
|
If we want to go with this, I also need to write some unit tests. ;) |
private static Aggregation getRegisteredAggregation(InstrumentDescriptor descriptor) { | ||
// todo look up based on fields of the descriptor. | ||
// todo: consider moving this method to its own class, for more targeted testing. | ||
private AggregationConfiguration findBestMatch(InstrumentDescriptor descriptor) { |
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.
Mostly makes sense but I'm wondering why is this a best match? Shouldn't the aggregation configuration be more precise?
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.
Well, I can imagine someone saying
- By default, aggregate all ValueRecorders with MinMaxSumCount.
- This particular ValueRecorder, named "http.request.latency"...that one I want Histograms for
So, "best match" will find the most precise selection and choose that, falling back to less precise ones. Is there a better name I could use?
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'd just name the method getAggregation
and the loop can have a comment // See if user defined a specific configuration for this descriptor
, the definition itself seems precise, not best-effort.
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.
Comment is just a nit, LGTM. Is there more work on this PR?
private static Aggregation getRegisteredAggregation(InstrumentDescriptor descriptor) { | ||
// todo look up based on fields of the descriptor. | ||
// todo: consider moving this method to its own class, for more targeted testing. | ||
private AggregationConfiguration findBestMatch(InstrumentDescriptor descriptor) { |
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'd just name the method getAggregation
and the loop can have a comment // See if user defined a specific configuration for this descriptor
, the definition itself seems precise, not best-effort.
If we think this is viable, I need to write a bunch of unit tests. I was holding off to get some feedback on the general approach before finishing that work. |
89937a9
to
9b59559
Compare
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Show resolved
Hide resolved
Co-authored-by: Anuraag Agrawal <[email protected]>
…ication.java Co-authored-by: Anuraag Agrawal <[email protected]>
…ication.java Co-authored-by: Anuraag Agrawal <[email protected]>
…ication.java Co-authored-by: Anuraag Agrawal <[email protected]>
…ication.java Co-authored-by: Anuraag Agrawal <[email protected]>
9ff19d1
to
77f5c7f
Compare
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/Batcher.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public int hashCode() { | ||
int result = descriptor != null ? descriptor.hashCode() : 0; |
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.
Think you can use Objects.hashCode
instead of your own null checks
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 just had IDEA generate this code. Hate to mess with generated code in general.
We should document what we want to do with generated equals/hashcode/toString implementations, so we don't have to discuss it. :)
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
Outdated
Show resolved
Hide resolved
sdk/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
Show resolved
Hide resolved
sdk/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Outdated
Show resolved
Hide resolved
sdk/src/main/java/io/opentelemetry/sdk/metrics/view/AggregationConfiguration.java
Outdated
Show resolved
Hide resolved
….java Co-authored-by: Anuraag Agrawal <[email protected]>
…gregationConfiguration.java Co-authored-by: Anuraag Agrawal <[email protected]>
…gregationConfiguration.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…nConfiguration.java Co-authored-by: Anuraag Agrawal <[email protected]>
…nConfiguration.java Co-authored-by: Anuraag Agrawal <[email protected]>
…gregationConfiguration.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
…strumentSelector.java Co-authored-by: Anuraag Agrawal <[email protected]>
sdk/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java
Outdated
Show resolved
Hide resolved
…strumentSelector.java
I'm going to merge, and we can iterate on this as we move forward with the metrics implementation. |
Note: related OTEP: open-telemetry/oteps#126
This first-cut supports the following:
select instruments to customize by instrument type and name.
specify the aggregation and the temporality of the aggregation to be used.
An example of how you could use it: