Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* #45520

lasteris · 2025-01-11T12:00:02Z

github-actions · 2025-01-11T12:27:55Z

🙈 The PR is closed and the preview is expired.

lasteris · 2025-01-11T14:26:16Z

Realized just now, that my solution also can be used via QuarkusRestClientBuilder.newBuilder()
.property(QuarkusRestClientProperties.LOGGING, ...) directly by users.
where ... is some implementation of RestClientsConfig.RestClientLoggingConfig interface :)

docs/src/main/asciidoc/rest-client.adoc

geoand · 2025-01-13T07:11:06Z

...ent/runtime/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientBuilderImpl.java

+        RestClientsConfig.RestClientLoggingConfig logging = getConfiguration().hasProperty(QuarkusRestClientProperties.LOGGING)
+                ? (RestClientsConfig.RestClientLoggingConfig) getConfiguration()
+                        .getProperty(QuarkusRestClientProperties.LOGGING)
+                : restClients.logging();


I can't say I understand this. I read the comment in the discussion, but I still don't understand why it's necessary

I'm just not sure it's correct to read a property and try to cast it to a specific type if the property is not really exists in map here. I thought that this way is more safe

You are about has property or about ternary ?

RestClientsConfig.RestClientLoggingConfig logging = (RestClientsConfig.RestClientLoggingConfig) getConfiguration() .getProperty(QuarkusRestClientProperties.LOGGING) : restClients.logging();

This is safe ?

My point is that I don't understand why this specific was change was made in the first place. What didn't work with the previous code?

My point is that I don't understand why this specific was change was made in the first place. What didn't work with the previous code?

Maybe i am wrong...

But as i understand previous code it reads only Global config for all clients (e. g . quarkus.rest-client.logging, not quarkus.rest-client."name".logging), not specific. All specific properties read from getConfiguration().getProperty... (which previously set via quarkusrestclientbuildercdidelegate) am i wrong ?

Sorry, i am trying to tell what i mean in another way :(
Previous code reads this:
io.quarkus.restclient.config.RestClientsConfig.logging()
but i wanted to propagate this:
io.quarkus.restclient.config.RestClientsConfig.clients[client].logging()

so, logging would work in the first place only for a specific declarative client, not for all like it works now

But as i understand previous code it reads only Global config for all clients (e. g . quarkus.rest-client.logging, not quarkus.rest-client."name".logging), not specific. All specific properties read from getConfiguration().getProperty... (which previously set via quarkusrestclientbuildercdidelegate) am i wrong ?

Correct, but the programmatic client doesn't have a name, so all it can do is read the global flag

Looking at what other config things do, I guess this is fine. But I would like to not pass the config object as I mention in https://github.com/quarkusio/quarkus/pull/45520/files#r1912958630

But as i understand previous code it reads only Global config for all clients (e. g . quarkus.rest-client.logging, not quarkus.rest-client."name".logging), not specific. All specific properties read from getConfiguration().getProperty... (which previously set via quarkusrestclientbuildercdidelegate) am i wrong ?

Correct, but the programmatic client doesn't have a name, so all it can do is read the global flag

Yeah! That's why I've done it via

trying to read named configuration

if no property has read from named config, trying to read global (for programmatic api support).
what am i missing ?

geoand · 2025-01-13T10:17:58Z

...time/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java

+    private void configureLogging(QuarkusRestClientBuilder builder) {
+        Optional<RestClientsConfig.RestClientLoggingConfig> logging = restClientConfig.logging();
+        if (logging.isPresent()) {
+            builder.property(QuarkusRestClientProperties.LOGGING, logging.get());
+        } else if (configRoot.logging() != null) {
+            builder.property(QuarkusRestClientProperties.LOGGING, configRoot.logging());
+        }
+    }


We usually don't pass entire config objects like this, but only specific properties.

We usually don't pass entire config objects like this, but only specific properties.

yeah, I saw that. But really there are no configs here, grouped by object, except multipart. So, I thought that my choice won't be wrong...
but if you said so, I need to rethink code / add separate constants for body-limit and logging-scope

I would prefer we have separate constants for scope and bodyLimit

good, I will do it then

@geoand last question for now
what do you think about these lines anyway:

else if (configRoot.logging() != null) { builder.property(QuarkusRestClientProperties.LOGGING, configRoot.logging()); }

maybe i should not try read global config thing, while it anyway would be read in RestClientBuilderImpl:

: restClients.logging();

Makes sense

...untime/src/main/java/org/jboss/resteasy/reactive/client/api/QuarkusRestClientProperties.java

geoand · 2025-01-14T09:22:12Z

...time/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java

+        restClientConfig.logging().ifPresent(logging -> {
+            builder.property(QuarkusRestClientProperties.LOGGING_SCOPE, logging.scope()
+                    .map(LoggingScope::forName)
+                    .orElse(LoggingScope.NONE));
+            builder.property(QuarkusRestClientProperties.LOGGING_BODY_LIMIT, logging.bodyLimit());
+        });


Can we get rid of the lambdas here please?

of course.
I've just thought, that it is better then get().somproperty.
is lambda blocked over all project ?

is lambda blocked over all project ?

Not strictly, it's just discouraged by some contributors (myself among them)

quarkus-bot · 2025-01-14T11:27:55Z

Status for workflow `Quarkus Documentation CI`

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

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

geoand

Thanks!

quarkus-bot · 2025-01-14T15:37:06Z

Status for workflow `Quarkus CI`

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

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

You can consult the Develocity build scans.

quarkus-bot bot added area/documentation area/rest area/rest-client area/resteasy-classic labels Jan 11, 2025

lasteris mentioned this pull request Jan 11, 2025

Logging configuration for specific declarative RestClient using named properties #45519

Closed

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Jan 11, 2025

geoand reviewed Jan 13, 2025

View reviewed changes

docs/src/main/asciidoc/rest-client.adoc Outdated Show resolved Hide resolved

geoand reviewed Jan 13, 2025

View reviewed changes

docs/src/main/asciidoc/rest-client.adoc Show resolved Hide resolved

geoand reviewed Jan 13, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

geoand reviewed Jan 13, 2025

View reviewed changes

lasteris force-pushed the QKS-45519 branch 2 times, most recently from f2b9547 to 1c22e49 Compare January 13, 2025 18:32

lasteris commented Jan 13, 2025

View reviewed changes

...untime/src/main/java/org/jboss/resteasy/reactive/client/api/QuarkusRestClientProperties.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

geoand reviewed Jan 14, 2025

View reviewed changes

Rest client per CDI injected client logging configuration

f12169b

lasteris force-pushed the QKS-45519 branch from 1c22e49 to f12169b Compare January 14, 2025 10:32

geoand approved these changes Jan 14, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

geoand merged commit c0b9484 into quarkusio:main Jan 14, 2025
35 checks passed

quarkus-bot bot added this to the 3.18 - main milestone Jan 14, 2025

quarkus-bot bot added the kind/enhancement New feature or request label Jan 14, 2025

lasteris changed the title ~~Rest client per CDI injected client logging configuration~~ Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* Jan 14, 2025

geoand mentioned this pull request Jan 14, 2025

[Main] Unrecognized configuration key "quarkus.rest-client.logging.scope" #45584

Closed

lasteris deleted the QKS-45519 branch January 15, 2025 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* #45520

Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* #45520

lasteris commented Jan 11, 2025 •

edited

Loading

github-actions bot commented Jan 11, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

lasteris commented Jan 11, 2025 •

edited

Loading

geoand Jan 13, 2025

lasteris Jan 13, 2025

lasteris Jan 13, 2025

lasteris Jan 13, 2025

geoand Jan 13, 2025

lasteris Jan 13, 2025 •

edited

Loading

lasteris Jan 13, 2025 •

edited

Loading

geoand Jan 13, 2025

geoand Jan 13, 2025

lasteris Jan 13, 2025

This comment has been minimized.

This comment has been minimized.

geoand Jan 13, 2025

lasteris Jan 13, 2025

geoand Jan 13, 2025

lasteris Jan 13, 2025 •

edited

Loading

geoand Jan 13, 2025

lasteris Jan 13, 2025 •

edited

Loading

geoand Jan 13, 2025

This comment has been minimized.

This comment has been minimized.

geoand Jan 14, 2025

lasteris Jan 14, 2025

geoand Jan 14, 2025

quarkus-bot bot commented Jan 14, 2025

geoand left a comment

This comment has been minimized.

quarkus-bot bot commented Jan 14, 2025 •

edited by github-actions bot

Loading

Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* #45520

Logging configuration for specific declarative RestClient using named "client" properties aka quarkus.rest-client."client".logging.* #45520

Conversation

lasteris commented Jan 11, 2025 • edited Loading

github-actions bot commented Jan 11, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

lasteris commented Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasteris Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

lasteris Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasteris Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasteris Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quarkus-bot bot commented Jan 14, 2025

Status for workflow Quarkus Documentation CI

geoand left a comment

Choose a reason for hiding this comment

This comment has been minimized.

quarkus-bot bot commented Jan 14, 2025 • edited by github-actions bot Loading

Status for workflow Quarkus CI

lasteris commented Jan 11, 2025 •

edited

Loading

github-actions bot commented Jan 11, 2025 •

edited

Loading

lasteris commented Jan 11, 2025 •

edited

Loading

lasteris Jan 13, 2025 •

edited

Loading

lasteris Jan 13, 2025 •

edited

Loading

lasteris Jan 13, 2025 •

edited

Loading

lasteris Jan 13, 2025 •

edited

Loading

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Jan 14, 2025 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`