You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wankai123 (via GitHub)" <gi...@apache.org> on 2025/07/03 01:09:50 UTC
[PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
wankai123 opened a new pull request, #13353:
URL: https://github.com/apache/skywalking/pull/13353
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #13353:
URL: https://github.com/apache/skywalking/pull/13353#discussion_r2181960448
##########
oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/grpc/GRPCClient.java:
##########
@@ -54,6 +55,35 @@ public class GRPCClient implements Client, HealthCheckable {
private boolean enableHealthCheck = false;
+ private long initialDelay = 5; // Initial delay for health check in seconds
+
+ private long period = 20; // Period for health check in seconds
+
+ // The default health check runnable that checks the health of the gRPC channel.
+ private Runnable healthCheckRunnable = () -> {
+ if (getChannel() != null && !getChannel().isShutdown()) {
+ HealthGrpc.HealthBlockingStub healthStub = HealthGrpc.newBlockingStub(getChannel());
+ HealthCheckRequest request = HealthCheckRequest.newBuilder().setService("").build();
+ try {
+ HealthCheckResponse response = healthStub.check(request);
+ handleStateChange(response);
+ } catch (StatusRuntimeException s) {
+ if (s.getStatus().getCode() == Status.Code.UNIMPLEMENTED) {
+ log.warn("Health check is not implemented on the remote gRPC server, regard as healthy. Host: {}, Port: {}", getHost(), getPort());
+ healthChecker.health();
+ } else {
+ log.warn("Health check failed for gRPC channel. Host: {}, Port: {}", getHost(), getPort(), s);
+ healthChecker.unHealth(s);
+ }
+ } catch (Throwable t) {
+ log.warn("Health check failed for gRPC channel. Host: {}, Port: {}", getHost(), getPort(), t);
+ healthChecker.unHealth(t);
+ }
+ } else {
+ log.warn("gRPC channel is not available for health check. Host: {}, Port: {}", getHost(), getPort());
Review Comment:
if the channel is not available, I think we should mark it unhealthy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #13353:
URL: https://github.com/apache/skywalking/pull/13353#discussion_r2181464273
##########
oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/grpc/GRPCClient.java:
##########
@@ -114,32 +144,51 @@ public void registerChecker(final HealthChecker healthChecker) {
this.enableHealthCheck = true;
}
+ /**
+ * Override the default health check runnable with a custom one.
+ * Must override before calling connect()
+ * This can be used to provide a different health check logic.
+ *
+ * @param healthCheckRunnable The custom health check runnable.
+ * @param initialDelay Initial delay before the first health check.
+ * @param period Period between subsequent health checks.
+ */
+ public void overrideCheckerRunnable(final Runnable healthCheckRunnable, final long initialDelay, final long period) {
Review Comment:
And preferably use the builder pattern because we have more arguments now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #13353:
URL: https://github.com/apache/skywalking/pull/13353#discussion_r2181462965
##########
oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/grpc/GRPCClient.java:
##########
@@ -114,32 +144,51 @@ public void registerChecker(final HealthChecker healthChecker) {
this.enableHealthCheck = true;
}
+ /**
+ * Override the default health check runnable with a custom one.
+ * Must override before calling connect()
+ * This can be used to provide a different health check logic.
+ *
+ * @param healthCheckRunnable The custom health check runnable.
+ * @param initialDelay Initial delay before the first health check.
+ * @param period Period between subsequent health checks.
+ */
+ public void overrideCheckerRunnable(final Runnable healthCheckRunnable, final long initialDelay, final long period) {
Review Comment:
TO make sure this is called before calling connect, what about adding an argument in the constructor, with this we can avoid misuse in code level in stead of in comment/doc level.
Also, to make this override actually useful, I think we should make it a `BiConsumer<ManagedChannel, HealthChecker>`, instead of a `Runnable`, so caller can use the channel and healthChecker inside the lambda, like this
```java
GRPCClient c = new GRPCClient("", 1, (channel, healthChecker) -> {
// use the channel to send request
// use the healthChecker to mark the healthiness
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #13353:
URL: https://github.com/apache/skywalking/pull/13353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support custom GRPCClient health checker logic. Roll back score meaning in GraphQL health check API. [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13353:
URL: https://github.com/apache/skywalking/pull/13353#discussion_r2181287354
##########
oap-server/server-health-checker/src/main/java/org/apache/skywalking/oap/server/health/checker/provider/HealthCheckerProvider.java:
##########
@@ -100,18 +100,22 @@ public void onInitialized(final HealthCheckerConfig initialized) {
@Override public void notifyAfterCompleted() throws ServiceNotProvidedException, ModuleStartException {
ses.scheduleAtFixedRate(() -> {
StringBuilder unhealthyModules = new StringBuilder();
- AtomicBoolean hasUnhealthyModule = new AtomicBoolean(false);
+ AtomicDouble unhealthyModule = new AtomicDouble(0);
Stream.ofAll(collector.collect())
.flatMap(metricFamily -> metricFamily.samples)
.filter(sample -> metricsCreator.isHealthCheckerMetrics(sample.name))
.forEach(sample -> {
if (sample.value < 1) {
unhealthyModules.append(metricsCreator.extractModuleName(sample.name)).append(",");
Review Comment:
Appending a trailing comma in the `details` string may lead to an extra comma at the end; consider trimming or conditionally appending separators.
##########
oap-server/server-library/library-client/src/main/java/org/apache/skywalking/oap/server/library/client/grpc/GRPCClient.java:
##########
@@ -114,32 +144,45 @@ public void registerChecker(final HealthChecker healthChecker) {
this.enableHealthCheck = true;
}
+ /**
+ * Override the default health check runnable with a custom one.
+ * Must override before calling connect()
+ * This can be used to provide a different health check logic.
+ *
+ * @param healthCheckRunnable The custom health check runnable.
+ * @param initialDelay Initial delay before the first health check.
+ * @param period Period between subsequent health checks.
+ */
+ public void overrideCheckerRunnable(final Runnable healthCheckRunnable, final long initialDelay, final long period) {
Review Comment:
Validate that `initialDelay` and `period` are non-negative before scheduling to avoid unexpected scheduling behavior.
```suggestion
public void overrideCheckerRunnable(final Runnable healthCheckRunnable, final long initialDelay, final long period) {
if (initialDelay < 0) {
throw new IllegalArgumentException("initialDelay must be non-negative. Provided value: " + initialDelay);
}
if (period < 0) {
throw new IllegalArgumentException("period must be non-negative. Provided value: " + period);
}
```
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/query/type/HealthStatus.java:
##########
@@ -26,7 +26,7 @@
@Setter
@ToString
public class HealthStatus {
- // score == 1 means healthy, otherwise it's unhealthy.
+ // score == 0 means healthy and no unhealthy component or connection， otherwise it's unhealthy.
Review Comment:
The comment uses a Chinese comma `，`; replace with an English comma `,` for consistency.
```suggestion
// score == 0 means healthy and no unhealthy component or connection, otherwise it's unhealthy.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org