Fix hierarchical otlp props lookup, add Grafana traces check #42763

alesj · 2024-08-26T16:06:59Z

Hierarchy property lookup with DevServices is atm broken for OTLP / Observability,
since both - default and fallback - end-up with the same ordinal, hence default is used.
Where actually fallback should be used -- since it's defined (added to config) in the LgtmResource class.

e.g.

# default set on OtlpExporterTracesConfig
quarkus.otel.exporter.otlp.traces.protocol=grpc

vs.

quarkus.otel.exporter.otlp.protocol=http/protobuf

quarkus-bot · 2024-08-26T16:07:16Z

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

github-actions · 2024-08-26T16:40:59Z

🎊 PR Preview 60994bf has been successfully built and deployed to https://quarkus-pr-main-42763-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

brunobat · 2024-08-26T17:29:05Z

...ava/io/quarkus/opentelemetry/runtime/config/HierarchicalOTelConnectionConfigInterceptor.java

@@ -46,7 +46,7 @@ public HierarchicalOTelConnectionConfigInterceptor() {

    @Override
    public Iterator<String> iterateNames(final ConfigSourceInterceptorContext context) {
-        Set<String> names = new HashSet<>();
+        Set<String> names = new LinkedHashSet<>();


The LinkedHashmap maintains a predictable iteration order... Can't remember why it is used here anymore.

I added it now, before it was HashSet ... but the order wasn't the problem., so I guess we can go back to plain HashSet ... ?

yeah, let's go back. This has a performance penalty.

A? With all 1M checks, this is the penalty? :-)
At least we know how they are ordered in the first place.
But yeah, np ... we can go back.

quarkus-bot · 2024-08-26T19:23:35Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit 102e1c3.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot · 2024-08-26T19:32:52Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 102e1c3.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 21	`Build`	Failures	Logs	Raw logs	🔍
✖	JVM Tests - JDK 17 Windows	`Build`	Failures	Logs	Raw logs	🔍

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/opentelemetry/deployment 
! Skipped: extensions/liquibase-mongodb/deployment extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment and 49 more

📦 extensions/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterOverrideConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

⚙️ JVM Tests - JDK 21 #

- Failing: extensions/opentelemetry/deployment 
! Skipped: extensions/liquibase-mongodb/deployment extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment and 49 more

📦 extensions/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterOverrideConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/opentelemetry/deployment 
! Skipped: extensions/liquibase-mongodb/deployment extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment and 49 more

📦 extensions/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

✖ io.quarkus.opentelemetry.deployment.exporter.otlp.OtlpExporterOverrideConfigTest. - History - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:708)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
	at io.quarkus.runtime.Application.start(Application.java:101)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:305)

radcortez

If you set a default for a particular property which also has a fallback name, the default does have priority. This may seem strange, but in reality, defaults are handled as any other source in terms of configuration. There are a few reasons for this, that we can discuss separately :)

Anyway, my understanding is that you have defaults for both ...otlp.endpoint and ... otlp.traces.endpoint for documentation purposes. The goal is to fallback to the general ...otlp.endpoint if ... otlp.traces.endpoint has no value. In that case a single default for ...otlp.endpoint should be enough.

For documentation purposes, you can add a default for the expression equivalent ${...otlp.endpoint}, or remove the default entirely and reference the fallback rule in the javadoc.

I'll leave it to your consideration.

alesj · 2024-08-27T14:42:23Z

If you set a default for a particular property which also has a fallback name, the default does have priority. This may seem strange, but in reality, defaults are handled as any other source in terms of configuration. There are a few reasons for this, that we can discuss separately :)

Anyway, my understanding is that you have defaults for both ...otlp.endpoint and ... otlp.traces.endpoint for documentation purposes. The goal is to fallback to the general ...otlp.endpoint if ... otlp.traces.endpoint has no value. In that case a single default for ...otlp.endpoint should be enough.

No, that doesn't work -- that's the problem.
The otlp.traces.endpoint default wins over "default" otlp.endpoint.
Since the 1st one is really a default, the 2nd one is gained via DevServices config.
And from ConfigGenerationBuildStep::generateBuilders I see they both end-up in defaults, hence same ordinal.

radcortez · 2024-08-27T15:58:29Z

The otlp.traces.endpoint default wins over "default" otlp.endpoint.

Yes, that is expected. My suggestion was to remove all defaults from the specific endpoints and leave it in the general otlp.endpoint, so you always get that fallback.

radcortez · 2024-08-27T16:01:16Z

BTW, we should probably hold on these changes and look into: #42506 (comment).

radcortez · 2024-08-27T16:19:35Z

@radcortez I don't know this code but I'm under the impression that we are doing a lot of work even if the property is not affected by this interceptor at all:
https://github.com/quarkusio/quarkus/pull/41786/files#diff-1af08e35f64e1b7a5138776c1c830a3ac9782fadb3e07878a15d54257c73eb6dR48-R72
We should test it the property is part of the affected properties (or at least that it starts with a prefix).

Exactly. I was looking into that piece of code as well. It evaluates every configuration present in the list of names because the mapping function returns the original name even if there is no fallback.

We probably need to step back and gather the exact requirements of what we want here so we can think the best way to implement this. I believe I'vewritten the initial implementations of this piece, but that was when the relocate / fallback mechanism was missing some pieces. We need to evaluate if this code is still required.

radcortez · 2024-08-27T16:34:50Z

Yes, that is expected. My suggestion was to remove all defaults from the specific endpoints and leave it in the general otlp.endpoint, so you always get that fallback.

Ah, you reuse the same interface for global and specific. Ok, let me think about this, plus the performance stuff.

brunobat · 2024-08-27T17:35:14Z

Ideally, global 'otlp.endpoint' should be applied if 'otlp.traces.endpoint' was not set by the user. Same for the other global properties.
If we can do that with mapping configs on the default by setting ${otlp.endpoint}, I'm fine with it and we can remove the interceptor.

brunobat

@alesj can you please check if Roberto's PR fixes the current problem or if we still need this PR?

brunobat · 2024-09-09T11:12:23Z

@alesj should we close this one?

alesj · 2024-09-09T11:25:14Z

Sure.

…

On Mon, 9 Sep 2024 at 13:12, Bruno Baptista ***@***.***> wrote: @alesj <https://github.com/alesj> should we close this one? — Reply to this email directly, view it on GitHub <#42763 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACRA6H65G4BOCMHJNSQ7Y3ZVV7CZAVCNFSM6AAAAABNEJ4WHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZXHAZDOMBUGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

alesj requested a review from brunobat August 26, 2024 16:07

quarkus-bot bot added the area/tracing label Aug 26, 2024

alesj requested a review from radcortez August 26, 2024 16:07

This comment has been minimized.

Sign in to view

brunobat reviewed Aug 26, 2024

View reviewed changes

brunobat approved these changes Aug 26, 2024

View reviewed changes

alesj force-pushed the obs_port_bug1 branch 3 times, most recently from 2905e76 to 7b2d5df Compare August 26, 2024 18:25

Fix hierarchical otlp props lookup, add Grafana traces check

102e1c3

alesj force-pushed the obs_port_bug1 branch from 7b2d5df to 102e1c3 Compare August 26, 2024 18:51

radcortez reviewed Aug 27, 2024

View reviewed changes

radcortez mentioned this pull request Aug 27, 2024

OpenTelemetry traces and metrics config fallback to base #42814

Merged

brunobat requested changes Aug 28, 2024

View reviewed changes

gsmet closed this Sep 9, 2024

quarkus-bot bot added the triage/invalid This doesn't seem right label Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hierarchical otlp props lookup, add Grafana traces check #42763

Fix hierarchical otlp props lookup, add Grafana traces check #42763

alesj commented Aug 26, 2024

quarkus-bot bot commented Aug 26, 2024

This comment has been minimized.

github-actions bot commented Aug 26, 2024 •

edited

Loading

This comment has been minimized.

brunobat Aug 26, 2024

alesj Aug 26, 2024

brunobat Aug 27, 2024

alesj Aug 27, 2024

quarkus-bot bot commented Aug 26, 2024

quarkus-bot bot commented Aug 26, 2024 •

edited by github-actions bot

Loading

radcortez left a comment

alesj commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

brunobat commented Aug 27, 2024

brunobat left a comment

brunobat commented Sep 9, 2024

alesj commented Sep 9, 2024 via email

Fix hierarchical otlp props lookup, add Grafana traces check #42763

Fix hierarchical otlp props lookup, add Grafana traces check #42763

Conversation

alesj commented Aug 26, 2024

quarkus-bot bot commented Aug 26, 2024

This comment has been minimized.

github-actions bot commented Aug 26, 2024 • edited Loading

This comment has been minimized.

brunobat Aug 26, 2024

Choose a reason for hiding this comment

alesj Aug 26, 2024

Choose a reason for hiding this comment

brunobat Aug 27, 2024

Choose a reason for hiding this comment

alesj Aug 27, 2024

Choose a reason for hiding this comment

quarkus-bot bot commented Aug 26, 2024

Status for workflow Quarkus Documentation CI

quarkus-bot bot commented Aug 26, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Failing Jobs

Failures

⚙️ JVM Tests - JDK 17 #

📦 extensions/opentelemetry/deployment

⚙️ JVM Tests - JDK 21 #

📦 extensions/opentelemetry/deployment

⚙️ JVM Tests - JDK 17 Windows #

📦 extensions/opentelemetry/deployment

radcortez left a comment

Choose a reason for hiding this comment

alesj commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

radcortez commented Aug 27, 2024

brunobat commented Aug 27, 2024

brunobat left a comment

Choose a reason for hiding this comment

brunobat commented Sep 9, 2024

alesj commented Sep 9, 2024 via email

github-actions bot commented Aug 26, 2024 •

edited

Loading

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Aug 26, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`