Implement new SemConv exporter health metrics #7265
Conversation
|
Hey @JonasKunz give me a heads up when you think this is ready for review. Happy to start with a high level review while its still in draft! |
|
@jack-berg a high level review would be appreciated! I'm happy with the overall structure and would mostly polish from now on, so it would be great to get your feedback before I polish things we'll end up changing anyway. I've updated the PR comment to give an introduction on how I envision the feature implementation in general. |
# Conflicts: # exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterMetrics.java # exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/metrics/OtlpHttpMetricExporterBuilder.java # exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/metrics/OtlpGrpcMetricExporterBuilder.java
| private static final AttributeKey<Boolean> ATTRIBUTE_KEY_SUCCESS = booleanKey("success"); | ||
| /** | ||
| * This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
| * any time. |
There was a problem hiding this comment.
Out of curiosity did the build force this comment to pass?
| public enum HealthMetricLevel { | ||
| OFF, | ||
| LEGACY, | ||
| ON |
There was a problem hiding this comment.
What do you think about calling this something like InternalTelemetrySchemaVersion, and having options like:
DISABLEDLEGACY1_31- earliest semconv that included the schema we're aiming to produceLATEST- shortcut to always stay pinned to the latest
| * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change | ||
| * at any time. | ||
| */ | ||
| public class ExporterMetricsAdapter { |
There was a problem hiding this comment.
#nit: naming. What do you think about something like ExporterInstrumenter. Is a bit of a not to the Instrumeter concept in opentelemetry-java-instrumentation, and acts the common interface we use for instrumenting exporters.
| * Sets the {@link HealthMetricLevel} defining which self-monitoring metrics this exporter | ||
| * collects. | ||
| * | ||
| * @since 1.50.0 |
There was a problem hiding this comment.
Omit in PRs. I'll add the appropriate annotation when I'm prepping for the release.
| * | ||
| * @since 1.50.0 | ||
| */ | ||
| public ZipkinSpanExporterBuilder setHealthMetricLevel(HealthMetricLevel level) { |
There was a problem hiding this comment.
Do we want to entertain the idea of supporting multiple schemas? My inclination is to say no, and only support one at a time, but makes migration more difficult. Also, there's precedent of double reporting with semconv. For example:
http/dup - emit both the old and the stable HTTP and networking conventions, allowing for a seamless transition.
| new UpstreamGrpcSender<>( | ||
| MarshalerTraceServiceGrpc.newFutureStub(defaultGrpcChannel, null), | ||
| /* shutdownChannel= */ false, | ||
| 10, | ||
| Collections::emptyMap, | ||
| null), | ||
| MeterProvider::noop); | ||
| HealthMetricLevel.OFF, | ||
| ComponentId.generateLazy("upstream_grpc_exporter"), |
There was a problem hiding this comment.
Seems like we should have an enum for all the known component types.
| return this; | ||
| } | ||
|
|
||
| public HttpExporterBuilder<T> setComponentType(String componentType) { |
There was a problem hiding this comment.
This is only used in one place to support exportAsJson. I think we should consider disabling internal telemetry in that case rather than jump through hoops to support it. Ostensibly, we don't support OTLP http/json export at all.
| @@ -124,6 +135,16 @@ public HttpExporterBuilder<T> setMeterProvider(Supplier<MeterProvider> meterProv | |||
| return this; | |||
| } | |||
|
|
|||
| public HttpExporterBuilder<T> setHealthMetricLevel(HealthMetricLevel healthMetricLevel) { | |||
There was a problem hiding this comment.
I believe you need to include health metric level and the component type in the toString implementation. Same applies to GrpcExporterBuilder.
| result.succeed(); | ||
| return; | ||
| } | ||
|
|
||
| exporterMetrics.addFailed(numItems); | ||
| metricRecording.finishFailed("" + statusCode, requestAttributes); |
There was a problem hiding this comment.
#nit
| metricRecording.finishFailed("" + statusCode, requestAttributes); | |
| metricRecording.finishFailed(String.valueOf(statusCode), requestAttributes); |
| Attributes requestAttributes = | ||
| Attributes.builder().put(SemConvAttributes.HTTP_RESPONSE_STATUS_CODE, statusCode).build(); |
There was a problem hiding this comment.
#nit
| Attributes requestAttributes = | |
| Attributes.builder().put(SemConvAttributes.HTTP_RESPONSE_STATUS_CODE, statusCode).build(); | |
| Attributes requestAttributes = Attributes.of(SemConvAttributes.HTTP_RESPONSE_STATUS_CODE, (long) statusCode); |
Also, is there no good way to abstract building these attributes into the metricRecording API?
This PR implements the new, experimental exporter health metrics defined in semantic conventions. The other metrics will be added in follow-up PRs.
We already have some experimental health metrics in the SDK, which I intend to eventually replace with the new ones from semantic conventions. I'm envisioning the replacement process as follows:
on,off,legacyand maybe in the futureextendedfor opt-in attributes / metricslegacytoon, this will be a breaking changeThis PR is designed with the eventual removal of support for the legacy metrics in mind:
ExporterMetricsclassExporterMetricsAdapteris available which mimics theExporterMetricsapi, but can fallback to reporting the legacy metrics insteadThis will allow us to easily remove the legacy metrics later on: Simply replace the usages of
ExporterMetricsAdapterwithExporterMetricsand delete the adapter and related code.