Set extension properties directly #41244

brunobat · 2024-06-17T08:06:57Z

Detect which extensions (Otel, Micrometer OTLP registry) are being used and set properties accordingly.
Remove Quarkus prefix to service properties because they will not be used by the users.
Update documentation
Boy Scout stuff like removing lambda expressions from extension and use inner classes for test profiles.

There are breaking changes.
The following properties are removed because they are not needed anymore:

quarkus.otel-collector.url
quarkus.grafana.url

The extension will now detect if Micrometer OTLP registry or the OpenTelemetry extension are present and set the relevant properties for the user.

...ment/src/main/java/io/quarkus/observability/deployment/ObservabilityDevServiceProcessor.java

alesj · 2024-06-17T08:31:36Z

...ation-tests/observability-lgtm/src/test/java/io/quarkus/observability/test/LgtmTestBase.java


    @Test
    public void testTracing() {
        log.info("Testing Grafana ...");
        String response = RestAssured.get("/api/poke?f=100").body().asString();
        log.info("Response: " + response);
-        GrafanaClient client = new GrafanaClient("http://" + url, "admin", "admin");
+        GrafanaClient client = new GrafanaClient(endpoint, "admin", "admin");


Does this work w/o http:// ?

Not sure, I'm just trying to make things compatible with the previous version.

Did you check / fix this?

the endpoint already includes the http:// part

github-actions · 2024-06-17T08:36:54Z

🙈 The PR is closed and the preview is expired.

brunobat · 2024-06-18T14:47:41Z

@alesj @melloware The PR is ready for review

melloware

LGTM

alesj · 2024-06-24T07:48:21Z

...e-common/src/main/java/io/quarkus/observability/devresource/DevResourceLifecycleManager.java

+     * @param catalog observability catalog. If OpenTelemetry or Micrometer are enabled.
+     * @return module's config
+     */
+    T config(ModulesConfiguration configuration, ExtensionsCatalog catalog);


Have this one call deprecated by default -- so we don't break existing impls (if there are any ...).

Why to deprecate this one?... It's the new method.

Not this one -- deprecate the old one.
And have the new one call the old one by default.

I thought about it, but they are needed for different things and I didn't want to mess things too much.

True ... but this way we can don't break any existing ones (if there are some :-) ...

alesj · 2024-06-24T07:50:11Z

And perhaps add prop names changes to CLI `update command?

alesj · 2024-06-24T07:51:51Z

And perhaps add prop names changes to CLI `update command?

... and we definitely need to mention the name changes in the release notes ...

brunobat · 2024-07-03T13:56:00Z

e definitely need to mention the name changes in the release notes ...

I will. I'm not sure about the update command, can you please do it later?

melloware

this looks great

gsmet · 2024-07-03T16:26:32Z

Hmmm, why are we having properties outside of the quarkus. namespace? We worked hard to normalize it, I'm not exactly excited by the idea of not following this convention.

melloware · 2024-07-03T16:28:47Z

@gsmet i think the properties in question can't be changed by user they are dynamically allocated by the system right?

brunobat · 2024-07-03T16:31:16Z

correct @melloware.
The property values are generated by the extension and should be read only.
In normal cases the user don't even need them and this PR is to make sure they are automatically configured into the extensions that need them.

geoand · 2024-07-04T06:30:04Z

Even in light of that explanation, I still don't understand why the properties are not under the Quarkus namespace.
Also, what does "read only" mean? IIUC from the docs, they are settable by the user aren't they?

brunobat · 2024-07-04T07:59:46Z

The long explanation...
Those properties are set by the extension with the outside ports of the docker container. They should not be changed by the user, therefore they are "read only".
If we place them under the quarkus namespace I have to create configurations for them, to remove the warning, and to make sure the user doesn't configure them, there are ways but it's not strait-forward and all this sounds overkill for properties that shouldn't even be public.
There's a case for not have them documented at all, however I decided to keep them because people can use those properties to interact with the container in creative ways.

geoand · 2024-07-04T08:02:35Z

If we place them under the quarkus namespace I have to create configurations for them, to remove the warning, and to make sure the user doesn't configure them, there are ways but it's not strait-forward and all this sounds overkill for properties that shouldn't even be public.

Sure, but this is the same argument that people have always used and we always push back against 😉

brunobat · 2024-07-04T08:06:45Z

Ok, then I will just say the properties were removed and are not available anymore.

geoand · 2024-07-04T08:11:08Z

👍🏼

brunobat · 2024-07-04T09:41:25Z

@gsmet removed references to those properties.

quarkus-bot · 2024-07-04T09:59:54Z

Status for workflow `Quarkus CI`

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

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

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

You can consult the Develocity build scans.

quarkus-bot · 2024-07-04T10:02:11Z

Status for workflow `Quarkus Documentation CI`

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

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

maxandersen · 2024-07-08T08:11:33Z

Remove Quarkus prefix to service properties because they will not be used by the users.

makes me squint seeing we remove quarkus prefix...aren't that prefix used for various config setup? ...is this because these properties are truly devmode internal only / used to decouple different services? should we maybe have another prefix for them or?

brunobat · 2024-07-08T10:22:16Z

Remove Quarkus prefix to service properties because they will not be used by the users.

makes me squint seeing we remove quarkus prefix...aren't that prefix used for various config setup? ...is this because these properties are truly devmode internal only / used to decouple different services? should we maybe have another prefix for them or?

Yes @maxandersen , they are internal properties that should not be changed by users and should not be documented. Therefore, the missing quarkus prefix because as soon as we add it, we need to create a property to not have a console warning and creating a property requires documentation.
As far as I know, this is the first time we have "read-only" properties like these ones. It would be nice to have a different prefix and behaviour for them. What do you think, @radcortez ?

radcortez · 2024-07-08T11:56:15Z

As far as I know, this is the first time we have "read-only" properties like these ones. It would be nice to have a different prefix and behaviour for them. What do you think, @radcortez ?

You can add such properties as Map<String, String> to a BUILD_AND_RUN_TIME_FIXED. Just pick a path for the Map. and then the validation will mark as valid any property name as part of the Map. It still requires documentation, but you can add something generic Internal Quarkus container properties... etc..

Yes, users could still set the configuration from build time, but since you can always override it, disregarding the one set by the user. When you get to dev mode, because the phase is BUILD_AND_RUN_TIME_FIXED, it cannot be changed.

brunobat · 2024-07-08T12:03:02Z

As far as I know, this is the first time we have "read-only" properties like these ones. It would be nice to have a different prefix and behaviour for them. What do you think, @radcortez ?

You can add such properties as Map<String, String> to a BUILD_AND_RUN_TIME_FIXED. Just pick a path for the Map. and then the validation will mark as valid any property name as part of the Map. It still requires documentation, but you can add something generic Internal Quarkus container properties... etc..

Yes, users could still set the configuration from build time, but since you can always override it, disregarding the one set by the user. When you get to dev mode, because the phase is BUILD_AND_RUN_TIME_FIXED, it cannot be changed.

That's why I prefer not even document the properties... Too much work for nothing. :)

alesj · 2024-07-12T09:29:33Z

@brunobat apart from those comments I just made, lgtm.

gsmet

Ok, let's make progress on this, we can still tweak things later if needed.

quarkus-bot bot added the area/documentation label Jun 17, 2024

alesj reviewed Jun 17, 2024

View reviewed changes

...ment/src/main/java/io/quarkus/observability/deployment/ObservabilityDevServiceProcessor.java Show resolved Hide resolved

alesj reviewed Jun 17, 2024

View reviewed changes

brunobat mentioned this pull request Jun 18, 2024

Observability Dev UI Card and Log Console #41281

Merged

brunobat force-pushed the lgtm-properties branch 2 times, most recently from 302306d to 5249b4d Compare June 18, 2024 14:41

brunobat marked this pull request as ready for review June 18, 2024 14:47

melloware approved these changes Jun 18, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

brunobat force-pushed the lgtm-properties branch from 5249b4d to cb1f5f6 Compare June 19, 2024 08:38

This comment has been minimized.

Sign in to view

brunobat requested a review from alesj June 19, 2024 08:42

This comment has been minimized.

Sign in to view

alesj reviewed Jun 24, 2024

View reviewed changes

alesj approved these changes Jun 24, 2024

View reviewed changes

brunobat mentioned this pull request Jun 26, 2024

Add observability-devservices-lgtm dependency to opentelmetry, to start devservices by default #41454

Open

brunobat force-pushed the lgtm-properties branch from cb1f5f6 to 7446960 Compare July 3, 2024 14:02

brunobat added the release/breaking-change label Jul 3, 2024

This comment has been minimized.

Sign in to view

brunobat requested review from melloware and alesj July 3, 2024 15:57

melloware approved these changes Jul 3, 2024

View reviewed changes

Set extension properties directly

10c335d

brunobat force-pushed the lgtm-properties branch from 7446960 to 10c335d Compare July 4, 2024 09:40

melloware approved these changes Jul 4, 2024

View reviewed changes

alesj approved these changes Jul 12, 2024

View reviewed changes

gsmet approved these changes Jul 12, 2024

View reviewed changes

gsmet merged commit 13ba085 into quarkusio:main Jul 12, 2024
23 checks passed

quarkus-bot bot added the kind/bugfix label Jul 12, 2024

quarkus-bot bot added this to the 3.13 - main milestone Jul 12, 2024

brunobat mentioned this pull request Jul 15, 2024

Configure OTel metrics on observability devservice #41903

Merged

Set extension properties directly #41244

Set extension properties directly #41244

Conversation

brunobat commented Jun 17, 2024 • 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

github-actions bot commented Jun 17, 2024 • edited Loading

brunobat commented Jun 18, 2024 • edited Loading

melloware left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

alesj Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

alesj commented Jun 24, 2024

alesj commented Jun 24, 2024

brunobat commented Jul 3, 2024

This comment has been minimized.

This comment has been minimized.

melloware left a comment

Choose a reason for hiding this comment

gsmet commented Jul 3, 2024

melloware commented Jul 3, 2024

brunobat commented Jul 3, 2024 • edited Loading

geoand commented Jul 4, 2024

brunobat commented Jul 4, 2024 • edited Loading

geoand commented Jul 4, 2024

brunobat commented Jul 4, 2024

geoand commented Jul 4, 2024

brunobat commented Jul 4, 2024

quarkus-bot bot commented Jul 4, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

quarkus-bot bot commented Jul 4, 2024

Status for workflow Quarkus Documentation CI

maxandersen commented Jul 8, 2024

brunobat commented Jul 8, 2024

radcortez commented Jul 8, 2024

brunobat commented Jul 8, 2024

alesj commented Jul 12, 2024

gsmet left a comment

Choose a reason for hiding this comment

brunobat commented Jun 17, 2024 •

edited

Loading

github-actions bot commented Jun 17, 2024 •

edited

Loading

brunobat commented Jun 18, 2024 •

edited

Loading

alesj Jul 12, 2024 •

edited

Loading

brunobat commented Jul 3, 2024 •

edited

Loading

brunobat commented Jul 4, 2024 •

edited

Loading

quarkus-bot bot commented Jul 4, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

Status for workflow `Quarkus Documentation CI`