Fix 'quarkus.otel.exporter.otlp.traces.timeout' default value

[mazenkhalil]

The default duration for timeout property is wrongly defined and causes runtime error.

[quarkus-bot]

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

[manofthepeace]

This should work once this gets merged; #32929 as I made the check case insensitive in the converter, also since Duration.parse does support both lowercase and uppercase.

[gsmet]

@mazenkhalil what type of error do you have? Because it's not the only duration specific like that and IIRC it works OK.

[brunobat]

Projects compile ok with this config.
What error do you see?
If we are going to perform this change, we also need to update HibernateSearchOutboxPollingRuntimeConfigPersistenceUnit and many other classes.

[mazenkhalil]

@gsmet @brunobat The following error appears on start only when quarkus.otel.traces.exporter is set to otlp instead of the default value cdi.

2023-05-15 16:21:04,101 ERROR [io.qua.run.Application] (Quarkus Main Thread) Failed to start application (with profile [dev]): io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException: Invalid duration property otel.exporter.otlp.traces.timeout=10S. Invalid duration string, found: S
	at io.opentelemetry.sdk.autoconfigure.spi.internal.DefaultConfigProperties.getDuration(DefaultConfigProperties.java:173)
	at io.opentelemetry.exporter.internal.otlp.OtlpConfigUtil.configureOtlpExporterBuilder(OtlpConfigUtil.java:101)
	at io.opentelemetry.exporter.otlp.internal.OtlpSpanExporterProvider.createExporter(OtlpSpanExporterProvider.java:37)
	at io.opentelemetry.sdk.autoconfigure.SpiUtil.lambda$loadConfigurable$0(SpiUtil.java:48)
	at io.opentelemetry.sdk.autoconfigure.NamedSpiManager.tryLoadImplementationForName(NamedSpiManager.java:46)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1708)
	at io.opentelemetry.sdk.autoconfigure.NamedSpiManager.getByName(NamedSpiManager.java:36)
	at io.opentelemetry.sdk.autoconfigure.SpanExporterConfiguration.configureExporter(SpanExporterConfiguration.java:91)
	at io.opentelemetry.sdk.autoconfigure.SpanExporterConfiguration.configureSpanExporters(SpanExporterConfiguration.java:66)
	at io.opentelemetry.sdk.autoconfigure.TracerProviderConfiguration.configureTracerProvider(TracerProviderConfiguration.java:51)
	at io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder.build(AutoConfiguredOpenTelemetrySdkBuilder.java:356)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer.getOpenTelemetry(OpenTelemetryProducer.java:139)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer_ProducerMethod_getOpenTelemetry_93822cfa4b2dfbcfc394a796cc51d5c75c33d0e9_Bean.doCreate(Unknown Source)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer_ProducerMethod_getOpenTelemetry_93822cfa4b2dfbcfc394a796cc51d5c75c33d0e9_Bean.create(Unknown Source)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer_ProducerMethod_getOpenTelemetry_93822cfa4b2dfbcfc394a796cc51d5c75c33d0e9_Bean.create(Unknown Source)
	at io.quarkus.arc.impl.AbstractSharedContext.createInstanceHandle(AbstractSharedContext.java:113)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:37)
	at io.quarkus.arc.impl.AbstractSharedContext$1.get(AbstractSharedContext.java:34)
	at io.quarkus.arc.impl.LazyValue.get(LazyValue.java:26)
	at io.quarkus.arc.impl.ComputingCache.computeIfAbsent(ComputingCache.java:69)
	at io.quarkus.arc.impl.AbstractSharedContext.get(AbstractSharedContext.java:34)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer_ProducerMethod_getOpenTelemetry_93822cfa4b2dfbcfc394a796cc51d5c75c33d0e9_Bean.get(Unknown Source)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryProducer_ProducerMethod_getOpenTelemetry_93822cfa4b2dfbcfc394a796cc51d5c75c33d0e9_Bean.get(Unknown Source)
	at io.quarkus.arc.impl.InstanceImpl.getBeanInstance(InstanceImpl.java:323)
	at io.quarkus.arc.impl.InstanceImpl.getInternal(InstanceImpl.java:307)
	at io.quarkus.arc.impl.InstanceImpl.get(InstanceImpl.java:188)
	at io.quarkus.opentelemetry.runtime.OpenTelemetryRecorder.createOpenTelemetry(OpenTelemetryRecorder.java:38)
	at io.quarkus.deployment.steps.OpenTelemetryProcessor$createOpenTelemetry624023256.deploy_0(Unknown Source)
	at io.quarkus.deployment.steps.OpenTelemetryProcessor$createOpenTelemetry624023256.deploy(Unknown Source)
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:111)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
	at io.quarkus.runner.GeneratedMain.main(Unknown Source)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.runner.bootstrap.StartupActionImpl$1.run(StartupActionImpl.java:104)
	at java.base/java.lang.Thread.run(Thread.java:833)

[brunobat]

Ok, that's the OTel SDK failing because it doesn't recognise the pattern.
Let's fix this.

[gsmet]

I'm not sure that's the right fix.

Isn't the problem that we do not format properly the duration we push to OTel? We need to support the user setting the duration to 5S. That's supported by Quarkus.

So somehow, we need to make sure that the Duration we have in this field is pushed to OTel in a format OTel can understand.

[gsmet]

@brunobat this comment is mostly for you ^

[brunobat]

@gsmet Ideally yes, that should be done around here: https://github.com/quarkusio/quarkus/blob/de16f435b6e3112fc73b0f032a98e0e838739dab/extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/OpenTelemetryProducer.java#LL157C26-L157C26

Or use converters in each prop... This would be cleaner.

However, support for the stock OTLP exporter is not yet official.
All other durations on the OTel extension already use the 's' instead of 'S'. We can see this a consistency issue and we can leave the support for 's' and 'S' for later.

(edited)

[gsmet]

Well, my problem with this is that we have a Duration in the Quarkus config and that means it can come in a lot of different forms. I'm a bit unsure about how you actually pass the info but the user can set any value that is a valid Duration. Or at least that's what we document and how it works everywhere else.

If OTel doesn't support that, we need to format the Duration in a way that is understandable by it.

Sure, we can merge this PR and fix the default value but that won't solve the problem for the user-defined values.

I will remove my change requested review for now but I think there's more to it and that it probably warrants a proper issue.

We can merge this workaround but we will need a proper fix.

[brunobat]

@gsmet created this: #33381

[gsmet]

Thanks for raising the issue and providing a PR!