eureka: allow overriding the healthCheckUrlPath #5369

codefromthecrypt · 2024-01-04T14:23:19Z

Motivation:

Zipkin has a custom health check service, in addition to the normal
one of type HealthCheckService. This is used to create a custom json
response and predated our use of Armeria. This custom one is mounted at
the path "/health" while the default is "/internal/health".

We recently added Eureka registration, but were not able to change the
path component without having to supply the scheme, host and port as
well. To work around this is a bit hacky, and I would like to remove it
as it cause log warnings.

This change is already supported upstream for similar reasons. See
https://github.com/Netflix/eureka/blob/2b09d77534eae703a36f79f99b27b4d6ed66c3f8/eureka-client/src/main/java/com/netflix/appinfo/InstanceInfo.java#L663-L666

Modifications:

Added a healthCheckUrlPath property to InstanceInfo and
corresponding builders, using a custom constructor to avoid
serializing it to JSON.
Modified to use this instead of the default path when set.
Added an integration test.

Result:

Health check paths are overridable.

codefromthecrypt · 2024-01-04T14:30:57Z

For context, here is our custom health check controller for /health

https://github.com/openzipkin/zipkin/blob/9cbe698b5bac7cc8ae89cec5d3aa237625fd3947/zipkin-server/src/main/java/zipkin2/server/internal/health/ZipkinHealthController.java#L46

The work around is to redundantly set the same path in config

https://github.com/openzipkin/zipkin/blob/master/zipkin-server/src/main/resources/zipkin-server-shared.yml#L225

just this isn't nice because it makes this log warning

zipkin  | 2024-01-04 14:26:25:588 [main] WARN RejectedRouteHandler - Virtual host '*' has a duplicate route: /health

p.s. I'll fix lint once I get an impression this would be an acceptable change.

codefromthecrypt · 2024-01-04T14:41:37Z

ps I can see in other tests that it is complicated to compare against the expected local host name. I'll fix that also, if someone nods the change is viable in principle.

github-actions · 2024-01-04T14:53:23Z

🔍 Build Scan® (commit: `468589a`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/2tlrdaxwgdqto
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/whyxbd7a6qdse
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/v3ekr4fcuo5wa
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/altnzlgkeefoa
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/m4ap5i3eofuim
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/nqdwbqquahd6i
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/tdnxjfaknnwh2
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/mfgzti4shfma2

codefromthecrypt · 2024-01-04T23:57:12Z

draft until decision is made (and I fix up is it is a go!)

minwoox

Hey, thanks for the PR. 😉
I'm so happy that you've returned to maintain Zipkin!

eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java

codefromthecrypt · 2024-01-09T23:23:32Z

ok ready to go. Thanks for the tips @minwoox!

minwoox

Left a couple of nits. Thanks!

minwoox · 2024-01-10T03:28:30Z

eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java

+                        InstanceStatus status,
+                        @Nullable String homePageUrl,
+                        @Nullable String statusPageUrl,
+                        @Nullable String healthCheckUrlPath, // Not in JSON


Global comment. How about just healthCheckPath?

oh I matched upstream after looking at it. I like healthCheckPath better so fine with changing it back. wdyt?

Ah, then let's leave it as is. Thanks!

eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java

minwoox · 2024-01-10T03:46:35Z

eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java

-                                secureHealthCheckUrl, oldInfo.getDataCenterInfo(),
-                                oldInfo.getLeaseInfo(), oldInfo.getMetadata());
+                                oldInfo.getHomePageUrl(), oldInfo.getStatusPageUrl(),
+                                oldInfo.getHealthCheckUrlPath(), healthCheckUrl, secureHealthCheckUrl,


What do you think of introducing two fields?

private final InstanceInfo originalInstanceInfo; @Nullable private InstanceInfo filledInstanceInfo;

so that we can call the original constructor of the InstanceInfo and make the newly-added one package-private?

I bet your suggestion will work.. lemme think about it and try

got closer.. I'll make another comment

codefromthecrypt

notes!

codefromthecrypt · 2024-01-10T23:26:31Z

eureka/src/main/java/com/linecorp/armeria/server/eureka/InstanceInfoBuilder.java

@@ -241,9 +251,10 @@ InstanceInfo build() {
        final LeaseInfo leaseInfo = new LeaseInfo(renewalIntervalSeconds, leaseDurationSeconds);
        return new InstanceInfo(instanceId, appName, appGroupName, hostname, ipAddr, vipAddress,


so the constructor needs to be accessible outside the package because interestingly the builder for instanceinfo is not in the same package as instanceinfo! Another way is to move this builder to common, then either re-use it here as-is, or extend or delegate to the common one.

Another way is to make a special copy factory function in instanceinfo like.. InstanceInfo.WithHealthCheckUrlPath

depends on what you feel is best (or something else, @minwoox!)

Oops, I missed it. 😓 Let's leave it as is.
I will make the constructor package-private after #4243 is merged.

codefromthecrypt · 2024-01-10T23:27:18Z

eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java

        final String defaultHostname = server.defaultHostname();
        final String hostName = oldInfo.getHostName() != null ? oldInfo.getHostName() : defaultHostname;
-        appName = oldInfo.getAppName() != null ? oldInfo.getAppName() : hostName;


this field mutation was indescribable in the function name, so I moved it out and made the function static to prevent a recurrence!

minwoox

Thanks, @codefromthecrypt! 😄

ikhoon

Looks nice! Thanks, @codefromthecrypt and @minwoox. 🙇‍♂️🙇‍♂️

ikhoon · 2024-01-23T08:00:42Z

eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java

+        final String baseURL = sessionProtocol.uriText() + "://" +
+                               hostnameOrIpAddr(hostnameOrIpAddr) + ':' + portWrapper.getPort();
+        if (oldHealthCheckUrlPath != null) {
+            return baseURL + oldHealthCheckUrlPath;


Minor) Should we check if oldHealthCheckUrlPath starts with / when the value is first set?

sure good idea

Motivation: Zipkin has a custom health check service, in addition to the normal one of type `HealthCheckService`. This is used to create a custom json response and predated our use of Armeria. This custom one is mounted at the path "/health" while the default is "/internal/health". We recently added Eureka registration, but were not able to change the path component without having to supply the scheme, host and port as well. To work around this is a bit hacky, and I would like to remove it as it cause log warnings. This change is already supported upstream for similar reasons. See https://github.com/Netflix/eureka/blob/2b09d77534eae703a36f79f99b27b4d6ed66c3f8/eureka-client/src/main/java/com/netflix/appinfo/InstanceInfo.java#L663-L666 Modifications: - Added a `healthCheckUrlPath` property to `InstanceInfo` and corresponding builders, using a custom constructor to avoid serializing it to JSON. - Modified to use this instead of the default path when set. - Added an integration test. Result: - Health check paths are overridable. Signed-off-by: Adrian Cole <adrian@tetrate.io>

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt · 2024-01-24T00:21:55Z

maybe there's a flakey test, can you kick 11's build?

ContentPreviewingFailureTest > shouldCompleteLogWhenExceptionIsThrown(String, int) > [3] path=/unexpected-exception, status=500 FAILED
    java.lang.AssertionError: 
    Expecting actual not to be null
        at com.linecorp.armeria.server.logging.ContentPreviewingFailureTest.shouldCompleteLogWhenExceptionIsThrown(ContentPreviewingFailureTest.java:100)

minwoox · 2024-01-24T01:07:00Z

maybe there's a flakey test, can you kick 11's build?

Oh, please don't worry about it. 😉

jrhee17

Looks good to me! Only left super minor comments regarding style. Thanks @codefromthecrypt 🙇 👍 🙇

eureka/src/main/java/com/linecorp/armeria/server/eureka/EurekaUpdatingListener.java

…UpdatingListener.java Co-authored-by: jrhee17 <guins_j@guins.org>

codefromthecrypt · 2024-01-24T03:01:15Z

thanks for the help @jrhee17!

minwoox · 2024-01-24T04:36:02Z

Thanks, @codefromthecrypt! 😄

…reka using path properties. Motivation: Following up on the improvements introduced in line#5369, it's beneficial to extend the autofill feature to include `homePageUrl` and `statusPageUrl` of an `InstanceInfo`. This enhancement streamlines configuration by automatically populating these URLs based on corresponding path properties. Modifications: - Introduce `homePageUrlPath` and `statusPageUrlPath` properties to `InstanceInfo` and their respective builders. - Implement autofill functionality for `homePageUrl` and `statusPageUrl` using the configured path properties. Result: - `homePageUrl` and `statusPageUrl` of an `InstanceInfo` in Eureka are correctly set if corresponding path properties are configured. - Close line#5464

…reka using path properties. (#5465) Motivation: Following up on the improvements introduced in #5369, it's beneficial to extend the autofill feature to include `homePageUrl` and `statusPageUrl` of an `InstanceInfo`. This enhancement streamlines configuration by automatically populating these URLs based on corresponding path properties. Modifications: - Introduce `homePageUrlPath` and `statusPageUrlPath` properties to `InstanceInfo` and their respective builders. - Implement autofill functionality for `homePageUrl` and `statusPageUrl` using the configured path properties. Result: - `homePageUrl` and `statusPageUrl` of an `InstanceInfo` in Eureka are correctly set if corresponding path properties are configured. - Close #5464

…reka using path properties. (line#5465) Motivation: Following up on the improvements introduced in line#5369, it's beneficial to extend the autofill feature to include `homePageUrl` and `statusPageUrl` of an `InstanceInfo`. This enhancement streamlines configuration by automatically populating these URLs based on corresponding path properties. Modifications: - Introduce `homePageUrlPath` and `statusPageUrlPath` properties to `InstanceInfo` and their respective builders. - Implement autofill functionality for `homePageUrl` and `statusPageUrl` using the configured path properties. Result: - `homePageUrl` and `statusPageUrl` of an `InstanceInfo` in Eureka are correctly set if corresponding path properties are configured. - Close line#5464

codefromthecrypt requested review from ikhoon, jrhee17, minwoox and trustin as code owners January 4, 2024 14:23

codefromthecrypt mentioned this pull request Jan 4, 2024

eureka: adds comment for follow-up regarding healthCheckPath openzipkin/zipkin#3672

Merged

codefromthecrypt marked this pull request as draft January 4, 2024 23:56

minwoox reviewed Jan 8, 2024

View reviewed changes

eureka/src/main/java/com/linecorp/armeria/internal/common/eureka/InstanceInfo.java Outdated Show resolved Hide resolved

codefromthecrypt force-pushed the eureka-healthCheckPath branch from 65c761d to c3f7aa5 Compare January 9, 2024 23:22

codefromthecrypt marked this pull request as ready for review January 9, 2024 23:22

codefromthecrypt changed the title ~~eureka: allow overriding the healthCheckPath~~ eureka: allow overriding the healthCheckUrlPath Jan 9, 2024

codefromthecrypt requested a review from minwoox January 9, 2024 23:23

minwoox reviewed Jan 10, 2024

View reviewed changes

codefromthecrypt commented Jan 10, 2024

View reviewed changes

minwoox added the new feature label Jan 11, 2024

minwoox approved these changes Jan 11, 2024

View reviewed changes

ikhoon approved these changes Jan 23, 2024

View reviewed changes

ikhoon added this to the 1.27.0 milestone Jan 23, 2024

codefromthecrypt added 3 commits January 24, 2024 06:56

progress

1713724

Signed-off-by: Adrian Cole <adrian@tetrate.io>

handle missing slash

957f312

Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt force-pushed the eureka-healthCheckPath branch from 744860b to 957f312 Compare January 23, 2024 23:45

jrhee17 approved these changes Jan 24, 2024

View reviewed changes

Update eureka/src/main/java/com/linecorp/armeria/server/eureka/Eureka…

0cd9396

…UpdatingListener.java Co-authored-by: jrhee17 <guins_j@guins.org>

codefromthecrypt and others added 2 commits January 24, 2024 11:00

Update eureka/src/main/java/com/linecorp/armeria/server/eureka/Eureka…

0e778af

…UpdatingListener.java Co-authored-by: jrhee17 <guins_j@guins.org>

Update eureka/src/main/java/com/linecorp/armeria/server/eureka/Eureka…

468589a

…UpdatingListener.java Co-authored-by: jrhee17 <guins_j@guins.org>

minwoox merged commit ac4b0e8 into line:main Jan 24, 2024
13 of 14 checks passed

codefromthecrypt deleted the eureka-healthCheckPath branch January 24, 2024 11:06

codefromthecrypt mentioned this pull request Feb 14, 2024

Add homePageUrlPath, statusPageUrlPath to EurekaUpdatingListenerBuilder #5464

Closed

minwoox mentioned this pull request Feb 15, 2024

Autofill homePageUrl and statusPageUrl of an InstanceInfo in Eureka using path properties. #5465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eureka: allow overriding the healthCheckUrlPath #5369

eureka: allow overriding the healthCheckUrlPath #5369

codefromthecrypt commented Jan 4, 2024 •

edited

codefromthecrypt commented Jan 4, 2024

codefromthecrypt commented Jan 4, 2024

github-actions bot commented Jan 4, 2024 •

edited

codefromthecrypt commented Jan 4, 2024

minwoox left a comment

codefromthecrypt commented Jan 9, 2024

minwoox left a comment

minwoox Jan 10, 2024

codefromthecrypt Jan 10, 2024

minwoox Jan 11, 2024

minwoox Jan 10, 2024

codefromthecrypt Jan 10, 2024

codefromthecrypt Jan 10, 2024

codefromthecrypt left a comment

codefromthecrypt Jan 10, 2024

minwoox Jan 11, 2024

codefromthecrypt Jan 10, 2024

minwoox left a comment

ikhoon left a comment

ikhoon Jan 23, 2024

codefromthecrypt Jan 23, 2024

codefromthecrypt Jan 23, 2024

codefromthecrypt commented Jan 24, 2024

minwoox commented Jan 24, 2024

jrhee17 left a comment

codefromthecrypt commented Jan 24, 2024

minwoox commented Jan 24, 2024

		@@ -241,9 +251,10 @@ InstanceInfo build() {
		final LeaseInfo leaseInfo = new LeaseInfo(renewalIntervalSeconds, leaseDurationSeconds);
		return new InstanceInfo(instanceId, appName, appGroupName, hostname, ipAddr, vipAddress,

eureka: allow overriding the healthCheckUrlPath #5369

eureka: allow overriding the healthCheckUrlPath #5369

Conversation

codefromthecrypt commented Jan 4, 2024 • edited

codefromthecrypt commented Jan 4, 2024

codefromthecrypt commented Jan 4, 2024

github-actions bot commented Jan 4, 2024 • edited

🔍 Build Scan® (commit: 468589a)

codefromthecrypt commented Jan 4, 2024

minwoox left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Jan 9, 2024

minwoox 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt 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

minwoox left a comment

Choose a reason for hiding this comment

ikhoon 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

codefromthecrypt commented Jan 24, 2024

minwoox commented Jan 24, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Jan 24, 2024

minwoox commented Jan 24, 2024

codefromthecrypt commented Jan 4, 2024 •

edited

github-actions bot commented Jan 4, 2024 •

edited

🔍 Build Scan® (commit: `468589a`)