Provide configuration properties for customising the auto-configured BatchSpanProcessor #44655
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
nosan
force-pushed
the
44644
branch
2 times, most recently
from
2bd37a6 to
6f84aa9
Compare
| */ | ||
| private final BatchSpanProcessor batchSpanProcessor = new BatchSpanProcessor(); | ||
|
|
||
| public BatchSpanProcessor getBatchSpanProcessor() { |
There was a problem hiding this comment.
I'd prefer a different naming. Just batch perhaps?
There was a problem hiding this comment.
batch sounds great, but I’m concerned about what might happen if OpenTelemetry
introduces a new feature also prefixed with BatchXxxYyy. In that case, batch could
become ambiguous.
There was a problem hiding this comment.
spanbatching? We try to not use dashes in parts before the final property name.
There was a problem hiding this comment.
How about .batchspan.xxx?
management.tracing.opentelemetry.batchspan.export-unsampled-spans batchspan
management.tracing.opentelemetry.batchspanprocessor.export-unsampled-spans batchspanprocessor
management.tracing.opentelemetry.spanbatching.export-unsampled-spans spanbatching
There was a problem hiding this comment.
I'm not sure, but in my opinion, batchspanprocessor seems like the most logical choice. Can't argue that it looks a bit ugly.
There was a problem hiding this comment.
However, these properties only impact the BatchSpanProcessor and cannot be applied, for instance, to the SimpleSpanProcessor or other processors.
Boot doesn't auto-configure the SimpleSpanProcessor so hopefully that confusion will not arise. The property names make no mention of processing so hopefully no one will expect them to apply to other processors either.
In my opinion, there should be some indication that these properties are intended for batch processing
Doesn't the max-batch-size property (that we omitted in the property examples above) do that?
The complete set of properties would be:
management.tracing.opentelemetry.span.export.include-unsampled
management.tracing.opentelemetry.span.export.max-batch-size
management.tracing.opentelemetry.span.export.max-queue-size
management.tracing.opentelemetry.span.export.schedule-delay
management.tracing.opentelemetry.span.export.timeout
I'm not totally sold on the prefix here. In part, it's the otlp vs opentelemetry problem again. I think this export may use Zipkin or OTLP depending on which of ZipkinSpanExporter, OtlpHttpSpanExporter, and OtlpGrpcSpanExporter has been auto-configured. We already have a couple of export-related properties for those:
management.otlp.tracing.export.enabledmanagement.zipkin.tracing.export.enabled
There was a problem hiding this comment.
Doesn't the max-batch-size property (that we omitted in the property examples above) do
that?
Fair enough 😄
I'm not totally sold on the prefix here. In part, it's the otlp vs opentelemetry problem
again.
I had the same feeling when I was choosing between the otlp and opentelemetry
prefixes but ended up using the latter.
I am curious whether .span. is needed at all, since OpenTelemetry exports only spans.
management.tracing.opentelemetry.export.include-unsampled
management.tracing.opentelemetry.export.max-batch-size
management.tracing.opentelemetry.export.max-queue-size
management.tracing.opentelemetry.export.schedule-delay
management.tracing.opentelemetry.export.timeoutThere was a problem hiding this comment.
I think opentelemetry is better here as this is configuring parts of the OpenTelemetry SDK. The protocol used for the export won't necessarily by OTLP so I think otlp would be inaccurate. This naming and its subtleties are more complex than we'd like but that complexity isn't of our making and there's only so much we can do to hide it.
+1 for dropping .span. from the names.
There was a problem hiding this comment.
@mhalbritter
What do you think? Do you have any concerns on your side?
There was a problem hiding this comment.
That sounds good to me. opentelemetry is the right prefix here, as it's not tied to OTLP. Dropping span is a good idea.
…ssor This commit adds the following properties to allow customization of the BatchSpanProcessor: - management.tracing.opentelemetry.export.include-unsampled - management.tracing.opentelemetry.export.timeout - management.tracing.opentelemetry.export.max-batch-size - management.tracing.opentelemetry.export.max-queue-size - management.tracing.opentelemetry.export.schedule-delay Default values were taken from BatchSpanProcessorBuilder. Signed-off-by: Dmytro Nosan <[email protected]>
See #44644