Copilot commented on code in PR #13382:
URL: https://github.com/apache/skywalking/pull/13382#discussion_r2227608490
##########
oap-server/server-query-plugin/status-query-plugin/src/main/java/org/apache/skywalking/oap/query/debug/DebuggingHTTPHandler.java:
##########
@@ -187,8 +192,10 @@ public String queryBasicTraces(@Param("service")
Optional<String> service,
@Param("pageNum") int pageNum,
@Param("pageSize") int pageSize
) {
+ // default to true if serviceLayer is not provided
Review Comment:
The comment should explain why the default value is `true` rather than just
stating what happens when the parameter is not provided. Consider explaining
that `true` represents normal services as opposed to virtual services.
```suggestion
// Default to true if serviceLayer is not provided, as true
represents normal services
// (as opposed to virtual services). This ensures the query operates
on normal services
// by default when no specific service layer is specified.
```
##########
oap-server/server-query-plugin/status-query-plugin/src/main/java/org/apache/skywalking/oap/query/debug/DebuggingHTTPHandler.java:
##########
@@ -474,9 +487,9 @@ public String queryLogs(@Param("service") Optional<String>
service,
coldStage.ifPresent(duration::setColdStage);
condition.setQueryDuration(duration);
}
-
+ final boolean isNormal = serviceLayer.map(s ->
Layer.nameOf(s).isNormal()).orElse(true);
Review Comment:
Consider extracting the repeated logic `serviceLayer.map(s ->
Layer.nameOf(s).isNormal()).orElse(true)` into a private helper method to
reduce code duplication across multiple methods.
##########
oap-server/server-query-plugin/status-query-plugin/src/main/java/org/apache/skywalking/oap/query/debug/DebuggingHTTPHandler.java:
##########
@@ -350,15 +357,17 @@ public String getServicesTopology(@Param("startTime")
String startTime,
@Param("endTime") String endTime,
@Param("step") String step,
@Param("coldStage") Optional<Boolean>
coldStage,
- @Param("serviceLayer") String
serviceLayer,
+ @Param("serviceLayer") Optional<String>
serviceLayer,
@Param("services") String services) {
Duration duration = new Duration();
duration.setStart(startTime);
duration.setEnd(endTime);
duration.setStep(Step.valueOf(step));
coldStage.ifPresent(duration::setColdStage);
+ // default to true if serviceLayer is not provided
Review Comment:
Similar to the previous comment, this should explain why `true` is the
appropriate default value for the `isNormal` flag.
```suggestion
// The default value of `true` for `isNormal` is chosen because most
services are expected to be normal
// unless explicitly specified otherwise. This ensures that the
majority of use cases are handled correctly
// without requiring additional input from the user.
```
##########
docs/en/debugging/query-tracing.md:
##########
@@ -440,12 +442,12 @@ debuggingTrace:
| step | The query step
| Yes |
| coldStage | Only for BanyanDB, the flag to query from cold stage,
default is false. | No |
| service | The service name
| Yes |
- | serviceLayer | The service layer name
| Yes |
+ | serviceLayer | The service layer name
| No |
Review Comment:
There's an inconsistency in the 'Required' column formatting. This line
shows 'No |' with extra spaces, while other lines in the same table show
'No |' with different spacing.
```suggestion
| serviceLayer | The service layer name
| No |
```
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
