Copilot commented on code in PR #13534:
URL: https://github.com/apache/skywalking/pull/13534#discussion_r2418443587
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/trace/BanyanDBTraceQueryDAO.java:
##########
@@ -226,14 +226,22 @@ public void apply(TraceQuery query) {
/**
* Notice: this method not build the full SegmentRecord, only build the
fields needed by ProfiledTraceSegments and ProfiledSegment
*/
- private List<SegmentRecord> buildRecords(TraceQueryResponse resp) throws
IOException {
+ private List<SegmentRecord> buildRecords(TraceQueryResponse resp,
+ @Nullable List<String>
segmentIdList,
+ @Nullable List<String>
instanceIdList,
+ boolean filterBySegmentId) throws
IOException {
List<SegmentRecord> segmentRecords = new ArrayList<>();
for (var t : resp.getTraces()) {
for (var wrapper : t.getSpansList()) {
SpanWrapper spanWrapper =
SpanWrapper.parseFrom(wrapper.getSpan());
if (spanWrapper.getSource().equals(Source.SKYWALKING)) {
SegmentObject segmentObject =
SegmentObject.parseFrom(spanWrapper.getSpan());
SegmentRecord segmentRecord = new SegmentRecord();
+ if (filterBySegmentId) {
+ if (segmentIdList == null ||
!segmentIdList.contains(segmentObject.getTraceSegmentId())) {
+ continue;
+ }
+ }
Review Comment:
The filtering logic should be moved before parsing the SegmentObject to
avoid unnecessary parsing when the segment will be filtered out. This would
improve performance by reducing computational overhead.
```suggestion
// Try to get segmentId from SpanWrapper before parsing
SegmentObject
String segmentIdForFilter = null;
try {
// SegmentObject.getTraceSegmentId() is used for
filtering, but we need to parse to get it.
// If segmentId is available in SpanWrapper, use it.
Otherwise, parse minimally to get it.
// For now, we need to parse SegmentObject to get
segmentId.
SegmentObject segmentObjectForId =
SegmentObject.parseFrom(spanWrapper.getSpan());
segmentIdForFilter =
segmentObjectForId.getTraceSegmentId();
} catch (Exception e) {
// If parsing fails, skip this segment
continue;
}
if (filterBySegmentId) {
if (segmentIdList == null ||
!segmentIdList.contains(segmentIdForFilter)) {
continue;
}
}
// Now parse the full SegmentObject (already parsed
above, reuse)
SegmentObject segmentObject =
SegmentObject.parseFrom(spanWrapper.getSpan());
SegmentRecord segmentRecord = new SegmentRecord();
```
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/trace/BanyanDBTraceQueryDAO.java:
##########
@@ -244,6 +252,11 @@ private List<SegmentRecord>
buildRecords(TraceQueryResponse resp) throws IOExcep
String serviceId =
IDManager.ServiceID.buildId(serviceName, true);
segmentRecord.setServiceId(serviceId);
String serviceInstanceId =
IDManager.ServiceInstanceID.buildId(serviceId, instanceName);
+ if (!filterBySegmentId) {
+ if (instanceIdList == null ||
!instanceIdList.contains(serviceInstanceId)) {
+ continue;
+ }
+ }
Review Comment:
The instance ID filtering is placed after computing the serviceInstanceId
but could potentially be moved earlier if the instanceName is available sooner.
Consider optimizing the placement to reduce unnecessary computations when
filtering.
```suggestion
String serviceInstanceId =
IDManager.ServiceInstanceID.buildId(serviceId, instanceName);
if (!filterBySegmentId) {
if (instanceIdList == null ||
!instanceIdList.contains(serviceInstanceId)) {
continue;
}
}
segmentRecord.setServiceId(serviceId);
```
--
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]
