Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. #5698

minwoox · 2024-05-24T04:14:05Z

Motivation:
Micrometer 1.13.0 updates its Prometheus dependency from 0.x to 1.x. In Prometheus 1.x, the package of PrometheusMeterRegistry has changed and CollectorRegistry is no longer used. More details can be found in the migration guide.

Modifications:

Updated Micrometer from 1.12.4 to 1.13.0.
Removed the Micrometer 1.3 integration test module.
- We dropped supporting Micrometer <= 1.5 already: Respect user-provided serviceLevelObjectives in timer histogram metrics #5661
Updated Prometheus from 0.16.0 to 1.3.0.
Added armeria-prometheus1 module.
- PrometheusMeterRegistries, PrometheusVersion1ExpositionService, and its builder classes are added.
Deprecated PrometheusMeterRegistries, PrometheusExpositionService, and its builder classes.

Result:

The older PrometheusMeterRegistries, PrometheusExpositionService, and its builders are deprecated.
- Use the same classes in the armeria-prometheus1.

Motivation: Micrometer 1.13.0 updates its Prometheus dependency from 0.x to 1.x. In Prometheus 1.x, the package of `PrometheusMeterRegistry` has changed and `CollectorRegistry` is no longer used. More details can be found in the [migration guide](https://github.com/micrometer-metrics/micrometer/wiki/1.13-Migration-Guide). Modifications: - Updated Micrometer from 1.12.4 to 1.13.0. - Removed the Micrometer 1.3 integration test module. - We dropped supporting Micrometer <= 1.5 already: line#5661 - Updated Prometheus from 0.16.0 to 1.3.0. - Added `PrometheusVersion1MeterRegistries`, `PrometheusVersion1ExpositionService`, and its builder classes. - Deprecated `PrometheusMeterRegistries`, `PrometheusExpositionService`, and its builder classes. Result: - New Prometheus version 1 classes are added to support the updated Micrometer dependency. - The older `PrometheusMeterRegistries`, `PrometheusExpositionService`, and its builders are deprecated.

github-actions · 2024-05-24T05:33:59Z

🔍 Build Scan® (commit: `eb77658`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/bw3lau45t2bdo
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/s5bf4rpsmgsbs
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/e7qm5nvdu5oac
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/melg6gp244i3m
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/c2cxbdggyhavw
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/2xs6uxoe2kngm
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/zfu76z2cpugig
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/k2ioguatwlyi2

jrhee17

Left a couple of minor comments, but looks good to me 👍 Thanks @minwoox 🙇 👍 🙇

jrhee17 · 2024-05-28T02:18:54Z

dependencies.toml

 [libraries.prometheus]
 module = "io.prometheus:simpleclient_common"
+version.ref = "prometheus-legacy"


Question) Is this used?

Suggested change

[libraries.prometheus]

module = "io.prometheus:simpleclient_common"

version.ref = "prometheus-legacy"

If it is used, should the version.ref be prometheus instead?

Oops, forgot to remove it. Thanks!

core/src/main/java/com/linecorp/armeria/server/metric/PrometheusVersion1ExpositionService.java

ikhoon · 2024-05-28T07:49:16Z

core/src/main/java/com/linecorp/armeria/common/metric/PrometheusVersion1MeterRegistries.java

+ * Provides the convenient factory methods for {@link PrometheusMeterRegistry}.
+ */
+@UnstableApi
+public final class PrometheusVersion1MeterRegistries {


Optional) It is not easy to find this new class to use the new API.
How about removing public static methods and adding an accessor to PrometheusMeterRegistries?

For example:

PrometheusMeterRegistries.V0.defaultRegistry() PrometheusMeterRegistries.V1.defaultRegistry()

We may deprecate all static methods in PrometheusMeterRegistries.

Same reason. 😉
#5698 (comment)

ikhoon · 2024-05-28T07:56:07Z

core/src/main/java/com/linecorp/armeria/server/metric/PrometheusVersion1ExpositionService.java

+        try (ByteBufOutputStream byteBufOutputStream = new ByteBufOutputStream(buffer)) {
+            final ExpositionFormatWriter writer = expositionFormats.findWriter(accept);
+            format = writer.getContentType();
+            writer.write(byteBufOutputStream, prometheusRegistry.scrape());
+            success = true;
+        } finally {


What do you think of consolidating this class with the original one? This will help users perform a more smooth migration.

class PrometheusExpositionService { @Deprecate public static PrometheusExpositionServiceBuilder builder(CollectorRegistry collectorRegistry) { ... } public static PrometheusExpositionServiceBuilder builder(PrometheusRegistry prometheusRegistry) { ... } }

What do you think of consolidating this class with the original one?

If so, users have to add two different modules even though they want the new one or the old one.
That's why I have created a separate class.

On second thought, it would be clearer to split PrometheusExpositionService and PrometheusMeterRegistries as a separate module such as :prometheus1. This direction is an Armeria way to support multiple versions for a library like other dependencies.

Alright, so you prefer @jrhee17's opinion: #5698 (comment)

spring/boot2-actuator-autoconfigure/build.gradle

jrhee17

Still looks good 👍 Left a minor question 🙇

jrhee17 · 2024-05-29T05:40:03Z

grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcMeterIdPrefixFunctionTest.java

@@ -48,7 +48,7 @@
 import com.linecorp.armeria.common.logging.RequestLogAccess;
 import com.linecorp.armeria.common.logging.RequestLogProperty;
 import com.linecorp.armeria.common.metric.MeterIdPrefix;
-import com.linecorp.armeria.common.metric.PrometheusMeterRegistries;
+import com.linecorp.armeria.common.prometheus.PrometheusMeterRegistries;


Question) I'm wondering if to minimize breaking changes, we could keep the package structure the same. I think this could be done by also declaring a separate prometheus0 module.

The upside would be:

Guaranteed one-line breaking change for users since they only need to add a dependency. (no need to touch java code)

The symmetry between prometheus0 and prometheus1 is just easier to reason about.

Users using prometheus1 don't need code for prometheus0

I think this could be done by also declaring a separate prometheus0 module.

Had a chat about this and I don't want to add prometheus0 right now because we have to maintain the same classes in the core module and prometheus0 module.
Having said this, we might introduce prometheus0 later if we want to keep supporting Prometheus 0 dependency when we reach Armeria v2.0.

we could keep the package structure the same.

We can't do this without breaking change for now. 😢

ikhoon · 2024-05-29T06:14:24Z

spring/boot3-autoconfigure/src/main/java/com/linecorp/armeria/spring/InternalServices.java

            final boolean hasPrometheus = hasAllClasses(
                    prometheusMeterRegistryClassName,
-                    "io.prometheus.client.CollectorRegistry");
+                    "io.prometheus.metrics.model.registry.PrometheusRegistry");


:prometheus1 is a compile scope that we need to check in runtime if it exists.

Suggested change

"io.prometheus.metrics.model.registry.PrometheusRegistry");

"io.prometheus.metrics.model.registry.PrometheusRegistry",

"com.linecorp.armeria.server.prometheus.PrometheusExpositionService");

I missed this. Added thanks! 😉

spring/boot2-actuator-autoconfigure/build.gradle

ikhoon · 2024-05-29T06:21:43Z

spring/boot3-actuator-autoconfigure/build.gradle

@@ -3,8 +3,8 @@ dependencies {
    api libs.jakarta.inject
    compileOnly libs.jakarta.validation

-    // TODO(anuraaga): Consider removing these since this module does not have related functionality.
-    optionalApi libs.micrometer.prometheus
+    optionalApi project(':prometheus1')


Users may not know they have to add :prometheus1 dependency to use Prometheus exposition. What do you think of advertising for prometheus1 where PrometheusExpositionService is linked in ArmeriaSettings?

That's a good idea. 👍

ikhoon

Looks good! 👍

… older classes. (line#5698) Motivation: Micrometer 1.13.0 updates its Prometheus dependency from 0.x to 1.x. In Prometheus 1.x, the package of `PrometheusMeterRegistry` has changed and `CollectorRegistry` is no longer used. More details can be found in the [migration guide](https://github.com/micrometer-metrics/micrometer/wiki/1.13-Migration-Guide). Modifications: - Updated Micrometer from 1.12.4 to 1.13.0. - Removed the Micrometer 1.3 integration test module. - We dropped supporting Micrometer <= 1.5 already: line#5661 - Updated Prometheus from 0.16.0 to 1.3.0. - Added `armeria-prometheus1` module. - `PrometheusMeterRegistries`, `PrometheusVersion1ExpositionService`, and its builder classes are added. - Deprecated `PrometheusMeterRegistries`, `PrometheusExpositionService`, and its builder classes. Result: - The older `PrometheusMeterRegistries`, `PrometheusExpositionService`, and its builders are deprecated. - Use the same classes in the `armeria-prometheus1`.

minwoox added dependencies deprecation labels May 24, 2024

minwoox added this to the 1.29.0 milestone May 24, 2024

minwoox requested review from ikhoon, jrhee17 and trustin as code owners May 24, 2024 04:14

jrhee17 approved these changes May 28, 2024

View reviewed changes

ikhoon reviewed May 28, 2024

View reviewed changes

minwoox added 3 commits May 29, 2024 09:59

Address the comment from @jrhee17

e070137

Add the prometheus1 module

8b2acee

Fix

8483711

minwoox changed the title ~~Add Prometheus classes for Version 1 and deprecate older classes.~~ Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. May 29, 2024

ikhoon reviewed May 29, 2024

View reviewed changes

spring/boot2-actuator-autoconfigure/build.gradle Show resolved Hide resolved

jrhee17 approved these changes May 29, 2024

View reviewed changes

ikhoon reviewed May 29, 2024

View reviewed changes

minwoox added 3 commits May 29, 2024 16:32

Merge branch 'main' into update_deps

ff9ede9

Address comments from @ikhoon

402e768

Fix

eb77658

ikhoon approved these changes May 31, 2024

View reviewed changes

minwoox merged commit 00d421f into line:main Jun 3, 2024
15 checks passed

minwoox deleted the update_deps branch June 3, 2024 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. #5698

Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. #5698

minwoox commented May 24, 2024 •

edited

Loading

github-actions bot commented May 24, 2024 •

edited

Loading

jrhee17 left a comment

jrhee17 May 28, 2024

minwoox May 29, 2024

ikhoon May 28, 2024

minwoox May 29, 2024

ikhoon May 28, 2024 •

edited

Loading

minwoox May 29, 2024

ikhoon May 29, 2024

minwoox May 29, 2024

jrhee17 left a comment

jrhee17 May 29, 2024

minwoox May 29, 2024

ikhoon May 29, 2024 •

edited

Loading

minwoox May 29, 2024

ikhoon May 29, 2024

minwoox May 29, 2024

ikhoon left a comment

	[libraries.prometheus]
	module = "io.prometheus:simpleclient_common"
	version.ref = "prometheus-legacy"

	"io.prometheus.metrics.model.registry.PrometheusRegistry");
	"io.prometheus.metrics.model.registry.PrometheusRegistry",
	"com.linecorp.armeria.server.prometheus.PrometheusExpositionService");

Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. #5698

Add armeria-prometheus1 module for Prometheus version 1 and deprecate older classes. #5698

Conversation

minwoox commented May 24, 2024 • edited Loading

github-actions bot commented May 24, 2024 • edited Loading

🔍 Build Scan® (commit: eb77658)

jrhee17 left a 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

Choose a reason for hiding this comment

ikhoon May 28, 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

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon May 29, 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

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented May 24, 2024 •

edited

Loading

github-actions bot commented May 24, 2024 •

edited

Loading

🔍 Build Scan® (commit: `eb77658`)

ikhoon May 28, 2024 •

edited

Loading

ikhoon May 29, 2024 •

edited

Loading