-
Notifications
You must be signed in to change notification settings - Fork 877
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
Finish wiring exemplars into metrics SDK #3599
Changes from 17 commits
3edcd58
e82f67f
1923c3b
bff4e35
fdd7a0a
3097982
3e0850e
0d0179b
357061b
abb302d
0f79ce9
142628f
22c60f8
a2f22bb
45d6566
de511cb
b63f049
620603c
e732205
a2a34fb
7ee88f9
2e0a585
0b9a0f4
e1d1565
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,4 +23,6 @@ dependencies { | |
testImplementation(project(":sdk:metrics-testing")) | ||
testImplementation(project(":sdk:testing")) | ||
testImplementation("com.google.guava:guava") | ||
|
||
jmh(project(":sdk:trace")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be better to put the benchmarks in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. JMH for trace also uses metrics sdk right now: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/build.gradle.kts#L37 This is in-line with our status-quo, but if you want to change that, I'd suggest outside this PR and we do it consistently. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
import io.opentelemetry.sdk.common.Clock; | ||
import io.opentelemetry.sdk.internal.ComponentRegistry; | ||
import io.opentelemetry.sdk.metrics.data.MetricData; | ||
import io.opentelemetry.sdk.metrics.exemplar.ExemplarSampler; | ||
import io.opentelemetry.sdk.metrics.exemplar.ExemplarFilter; | ||
import io.opentelemetry.sdk.metrics.export.MetricProducer; | ||
import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState; | ||
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistry; | ||
|
@@ -42,9 +42,9 @@ public final class SdkMeterProvider implements MeterProvider, MetricProducer { | |
private final MeterProviderSharedState sharedState; | ||
|
||
SdkMeterProvider( | ||
Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarSampler exemplarSampler) { | ||
Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarFilter exemplarFilter) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, I like Filter a lot better since we apply reservoir sampling after it anyways. Perhaps it allows renaming what we have as ExemplarReservoir to ExemplarFilter :-D |
||
this.sharedState = | ||
MeterProviderSharedState.create(clock, resource, viewRegistry, exemplarSampler); | ||
MeterProviderSharedState.create(clock, resource, viewRegistry, exemplarFilter); | ||
this.registry = | ||
new ComponentRegistry<>( | ||
instrumentationLibraryInfo -> new SdkMeter(sharedState, instrumentationLibraryInfo)); | ||
|
This file was deleted.
This file was deleted.
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.
Not for this PR since we're correctly reflecting the spec, but all of our other enum-type SDK config uses lowercase, not uppercase, would be nice if it can be fixed @jsuereth I don't know if we have time before the release 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.
This is a reasonable thing to fix. If all other spec uses lower case, that's an easy bugfix. We're only in feature-freeze right now.
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.
If it's ok, I'll fix this in a follow up PR once this is submitted.