Skip to content
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

OTEL resources attributes with interpolation doesn't get set #40968

Closed
luneo7 opened this issue Jun 4, 2024 · 14 comments · Fixed by #41405
Closed

OTEL resources attributes with interpolation doesn't get set #40968

luneo7 opened this issue Jun 4, 2024 · 14 comments · Fixed by #41405
Labels
area/tracing kind/bug Something isn't working
Milestone

Comments

@luneo7
Copy link
Contributor

luneo7 commented Jun 4, 2024

Describe the bug

OTEL resources attributes with interpolation doesn't get set.
Whenever we use an interpolation for something that doesn't resolve right the quarkus.otel.resource.attributes is not taken into account

Expected behavior

Properties that are runtime get evaluated during runtime, and quarkus.otel.resource.attributes gets set

Actual behavior

We see a warning:

[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.opentelemetry.tracer.resource-attributes" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

And property is not set

How to Reproduce?

Clone https://github.com/quarkusio/quarkus-quickstarts/blob/3.11/opentelemetry-quickstart/pom.xml OTEL Quick start and add this to the properties:

quarkus.otel.resource.attributes=service.environment=${DD_ENV:${current.env}},deployment.environment=${DD_ENV:${current.env}},service.version=${DD_VERSION:unset}

with that build natively and you will get:

❯ mvn clean package -Pnative -DskipTests

[WARNING] [io.quarkus.config] Unrecognized configuration key "quarkus.opentelemetry.tracer.resource-attributes" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo

run it with:

❯ export DD_ENV=bazinga
❯ export DD_VERSION=1.0-baz
❯ ./target/opentelemetry-quickstart-1.0.0-SNAPSHOT-runner -Dcurrent.env=asd

you can take a look at Jaeger and there won't be the custom resource attributes
image

....

If you checkout the 3.8.x quickstart

❯ git stash
❯ git checkout -b b3.8.4 tags/3.8.4
❯ git stash pop
❯ mvn clean package -Pnative -DskipTests
❯ export DD_ENV=bazinga
❯ export DD_VERSION=1.0-baz
❯ ./target/opentelemetry-quickstart-1.0.0-SNAPSHOT-runner -Dcurrent.env=asd

You won't see the warning log and it will work as expected

image

So it works up until 3.8.4... any Quarkus after that it doesn't work =]

Output of uname -a or ver

Darwin C02C32WQMD6R 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:54:10 PST 2023; root:xnu-10002.61.3~2/RELEASE_X86_64 x86_64

Output of java -version

openjdk version "21.0.3" 2024-04-16 LTS OpenJDK Runtime Environment Liberica-NIK-23.1.3-1 (build 21.0.3+10-LTS) OpenJDK 64-Bit Server VM Liberica-NIK-23.1.3-1 (build 21.0.3+10-LTS, mixed mode, sharing)

Quarkus version or git rev

3.11.0

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.5 (57804ffe001d7215b5e7bcb531cf83df38f93546) Maven home: /usr/local/Cellar/maven/3.9.5/libexec Java version: 21.0.2, vendor: GraalVM Community, runtime: /Library/Java/JavaVirtualMachines/graalvm-community-openjdk-21.0.2+13.1/Contents/Home Default locale: en_CA, platform encoding: UTF-8 OS name: "mac os x", version: "14.2.1", arch: "x86_64", family: "mac"

Additional information

No response

@luneo7 luneo7 added the kind/bug Something isn't working label Jun 4, 2024
@luneo7
Copy link
Contributor Author

luneo7 commented Jun 4, 2024

/cc @radcortez @brunobat

Copy link

quarkus-bot bot commented Jun 5, 2024

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

@geoand
Copy link
Contributor

geoand commented Jun 5, 2024

Although it's not solution to the problem, I really think the way quarkus.otel.resource.attributes is done in Quarkus is incorrect

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 5, 2024

@geoand how so? The way that I see is that Quarkus is trying to do the same thing as autoconfigure, as you can pass as an env variable, so it is runtime evaluated, you can have multiple resources implementing the SPI so all resources get merged into one single resource (https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#opentelemetry-resource). The difference is that in Quarkus you can also do that using cdi beans, which get merged into one resource as well.

@geoand
Copy link
Contributor

geoand commented Jun 5, 2024

Forcing users to use key1=val1,key2=val2,key3=val3 as a value is not a good experience.
It would be much better if there resourceAttributes was a Map<String, String> as is done in other places in Quarkus

@radcortez
Copy link
Member

Yes, this was to keep the same expected value format used by OTel, so we only need to rewrite the configuration names to pass the configuration back to OTel.

Ideally, our own Quarkus OTel configuration should follow our best practices. In this case, we need a conversion layer to convert our configuration values to the expected format of OTel. We could go ahead and revisit this discussion.

@brunobat
Copy link
Contributor

I agree with @radcortez this way we isolate users from potencial OTel side changes.
I don't see this as a priority and community contributions are welcome.

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 13, 2024

The only thing is that this issue isn't about how we inform the resource attributes 😆 but rather an interpolation/substitution issue 😊

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 13, 2024

If I do pass -Dcurrent.env=bazinga during the build phase it works as expected, the config gets recorded, but if that is not set, and we have that only during runtime, being current.env a fallback to DD_ENV, it doesn't work properly, and it used to work up until Quarkus 3.8

@brunobat
Copy link
Contributor

This are in OTel extension hasn't changed since 3.8.0 was released.
I wonder if this is a config issue, @radcortez?
@luneo7 can you please create a test to illustrate the problem?

@radcortez
Copy link
Member

It is most likely related to #39604.

We had to exclude all the recording of Environment Variables due to some possible dangerous behaviour. Check #39388.

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 19, 2024

Yeah, the only thing is that it is being evaluated/recorded during build time, seems to be triggering the fallback config source interceptor, relocating it to the old quarkus.opentelemetry root, and it doesn't work during runtime 😅

@radcortez
Copy link
Member

This broke with #39308, when we changed how relocates and fallbacks work. We could fix it with a change to the relocation / fallback interceptor, but I think it is time to remove them completely.

@brunobat
Copy link
Contributor

I Agree @radcortez. We need to remove these old properties. Created this: #41409

@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tracing kind/bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants