Copilot commented on code in PR #13807:
URL: https://github.com/apache/skywalking/pull/13807#discussion_r3055169652
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/storage/profiling/pprof/IPprofTaskLogQueryDAO.java:
##########
@@ -25,7 +25,11 @@
public interface IPprofTaskLogQueryDAO extends DAO {
/**
- * search all task log list in appoint task id
+ * Search task logs by the given task id.
+ *
+ * @param taskId the task id to filter by, must not be null or blank
+ * @return the task logs associated with the given task id
+ * @throws IOException if the query fails
*/
- List<PprofTaskLog> getTaskLogList() throws IOException;
+ List<PprofTaskLog> getTaskLogList(String taskId) throws IOException;
Review Comment:
The Javadoc states `taskId` must not be null or blank, but there is no
validation at the service/DAO boundary. This leads to inconsistent behavior
across implementations (e.g., Elasticsearch `Query.term` throws NPE on null,
while JDBC/BanyanDB may silently return empty results). Either enforce this
contract (preferred: validate and throw an `IllegalArgumentException` with a
clear message) or relax the Javadoc to match actual behavior.
##########
docs/en/changes/changes.md:
##########
@@ -13,6 +13,7 @@
* Fix duplicate `TABLE_COLUMN` condition in
`JDBCMetadataQueryDAO.findEndpoint()`, which was binding the same parameter
twice due to a copy-paste error.
* Support MCP (Model Context Protocol) observability for Envoy AI Gateway: MCP
metrics (request CPM/latency, method breakdown, backend breakdown,
initialization latency, capabilities), MCP access log sampling (errors only),
`ai_route_type` searchable log tag, and MCP dashboard tabs.
* Add weighted handler support to `BatchQueue` adaptive partitioning. MAL
metrics use weight 0.05 at L1 (vs 1.0 for OAL), reducing partition count and
memory overhead when many MAL metric types are registered.
+* Fix missing `taskId` filter in pprof task query and its
JDBC/BanyanDB/Elasticsearch implementations.
Review Comment:
CHANGES entry says “pprof task query”, but this PR updates the pprof *task
log* query/filtering. Consider renaming the entry to “pprof task log query” to
match the actual change and avoid confusion.
```suggestion
* Fix missing `taskId` filter in pprof task log query and its
JDBC/BanyanDB/Elasticsearch implementations.
```
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/profiling/pprof/PprofQueryService.java:
##########
@@ -105,15 +105,9 @@ public PprofStackTree queryPprofData(String taskId,
List<String> instanceIds) th
}
public List<PprofTaskLog> queryPprofTaskLogs(String taskId) throws
IOException {
- List<PprofTaskLog> taskLogList = getTaskLogQueryDAO().getTaskLogList();
- return findMatchedLogs(taskId, taskLogList);
- }
-
- private List<PprofTaskLog> findMatchedLogs(final String taskID, final
List<PprofTaskLog> allLogs) {
- return allLogs.stream()
- .filter(l -> Objects.equals(l.getId(), taskID))
- .map(this::extendTaskLog)
- .collect(Collectors.toList());
+ return getTaskLogQueryDAO().getTaskLogList(taskId).stream()
+ .map(this::extendTaskLog)
+ .collect(Collectors.toList());
Review Comment:
`taskId` is passed directly to the storage layer without validation. Since
the Elasticsearch implementation will throw on null (and the DAO Javadoc claims
non-null/non-blank), add an explicit check here (or earlier at the GraphQL
boundary) to fail fast with a clear error message and consistent behavior
across storage backends.
##########
oap-server/server-storage-plugin/storage-elasticsearch-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/elasticsearch/query/PprofTaskLogQueryEsDAO.java:
##########
@@ -48,13 +48,14 @@ public PprofTaskLogQueryEsDAO(ElasticSearchClient client,
int profileTaskQueryMa
}
@Override
- public List<PprofTaskLog> getTaskLogList() throws IOException {
+ public List<PprofTaskLog> getTaskLogList(String taskId) throws IOException
{
final String index =
IndexController.LogicIndicesRegister.getPhysicalTableName(PprofTaskLogRecord.INDEX_NAME);
final BoolQueryBuilder query = Query.bool();
if
(IndexController.LogicIndicesRegister.isMergedTable(PprofTaskLogRecord.INDEX_NAME))
{
query.must(Query.term(IndexController.LogicIndicesRegister.RECORD_TABLE_NAME,
PprofTaskLogRecord.INDEX_NAME));
}
-
+ query.must(Query.term(PprofTaskLogRecord.TASK_ID, taskId));
+
Review Comment:
`Query.term(..., taskId)` requires a non-null value and will throw a
`NullPointerException` if `taskId` is null. Add an explicit argument validation
(or ensure callers validate) so failures are consistent and produce a clearer
error than an NPE.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
