Add OTEL Config Params

[luneo7]

Adds additional config params for OpenTelemetry (addresses features requests in #17640).
This PR adds a way of setting resources, propagators and id generator from the OTEL library using application properties

[quarkus-bot]

/cc @kenfinnigan

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself.

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 6de4e4d

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	Reclaim Disk Space	⚠️ Check →	Logs	Raw logs

[kenfinnigan]

@luneo7 I plan to review this tomorrow, apologies for the delay.

I also wanted to let you know they've modified the Trace propagators to use the same SPI: open-telemetry/opentelemetry-java#3294

Once released, we should be able to load all propagators with a single SPI without any dependencies except the SPI one!

[luneo7]

Hi @kenfinnigan I think there is 3 ways of approaching this changes.
1 - Config params/properties
2 - Beans
3 - Mixing 1 & 2

When doing the PR I thought that it would be beneficial to everything be configured by params/properties because it would be more straight forward for the developer to get it running and configured, because those configs can be all set by the application properties and the user wouldn't need to code any class to start using it, and after reading the OTEL SDK autoconfigure extension they are doing this just reading properties.

There is one drawback though of doing everything by config params, Quarkus implementation would be tied to the OTEL implementation and when adding new features (resources, propagators etc) we would need to change the implementation if not using service loaders. I know that they are adding service loader for the propagators in a new release and we can also use it to construct the resources, but there is some stuff like samplers that can't be configured using service loader, Jaeger one for instance needs parameters to construct the class.

When using beans it makes the developer always have to code something to get it running, if we do that for resources and the user wants 3 resources, then there will be 3 producers, or just one merging all resources and returning it, and then we start to add complexity to the OTEL config. The benefit of it is that if something new appears in the OTEL lib or the user coded their own implementation for something they can use it right away.

And there is also the possibility of mixing app config with beans, if we do this we need to be sure that it makes sense, because the user might ask the why he is configuring some stuff in the app properties and needs to create a beans for other stuff.

What do you think about all this stuff =] ?

[kenfinnigan]

Unfortunately, I think we need a mixture, depending on the particular piece of information.

For the case of IdGenerator I think it's reasonable to look for a CDI Bean of that type, and if there isn't one then we use the default generator.

For Resource details it needs a combination of properties/env vars, and then service loader or CDI beans, to enable a mixture of build time specific Resource types but also allow per environment information.

Not sure I've answered all your questions, but hopefully I've articulated my current thinking

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 3a4903f

Status	Name	Step	Test failures	Logs	Raw logs
✖	Native Tests - Misc4	Build	⚠️ Check →	Logs	Raw logs

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 4fc6d31

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	JVM Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring	Reclaim Disk Space	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 7c7d3bd

Status	Name	Step	Test failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✖	JVM Tests - JDK 16	Build	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 integration-tests/kafka-streams

✖ io.quarkus.it.kafka.streams.KafkaStreamsPropertiesTest.testProperties - More details - Source on GitHub

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building dc8bd7a

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	Build	⚠️ Check →	Logs	Raw logs

[kenfinnigan]

I know this has been a lengthy back and forth, but I really appreciate the contribution and effort to have this included. It's saved me a lot of work

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 870fabc

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	JVM Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	Build	⚠️ Check →	Logs	Raw logs

[kenfinnigan]

If you could fix the ".z" in a comment and also squash all the commits down to 1, we will let CI run and see how it goes.

Thanks again for all the great work!

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 7af08fb

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Devtools Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	Devtools Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	Gradle Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	Gradle Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	Build	⚠️ Check →	Logs	Raw logs
⌛	Maven Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	Maven Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	MicroProfile TCKs Tests	Verify	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Amazon		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Cache		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data1	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data2	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data3	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data4	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data5	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data6	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data7	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Main	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 6b8e74a

Status	Name	Step	Test failures	Logs	Raw logs
⌛	MicroProfile TCKs Tests	Verify	⚠️ Check →	Logs	Raw logs

[kenfinnigan]

I'm going to review this today, but I wondered whether we need any updates to the guide around this new configuration? If you think we do, I'm fine with it being a separate PR so as not to trigger another CI run

[luneo7]

I'm glad to help =] The back & forth is part of it =]

So I think that it would be good to update the docs, a separate PR would be ideal

[luneo7]

@kenfinnigan while doing this PR I found something... the vertx tracing adapter is using httpServerRequest.remoteAddress() to set the HTTP_CLIENT_IP attribute, but I think we need to check if the X-Forwarded-For is present and use its value instead to show the actual client IP, doing that we will be matching the spec .... so it would be use the header value if present, defaulting to httpServerRequest.remoteAddress() if not

[kenfinnigan]

@luneo7 Yes we have an issue with it already: #17612

If you want to tackle that as a separate PR that would be fantastic!

[luneo7]

Just opened a PR for the client ip: #17958

[kenfinnigan]

Thanks again for all your work on this @luneo7!!