[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #257: [OAP Integration] Register missing TopN registry service

["hanahmily (via GitHub)" <gi...@apache.org>]

hanahmily commented on code in PR #257:
URL: https://github.com/apache/skywalking-banyandb/pull/257#discussion_r1133810234


##########
test/cases/measure/data/testdata/service_instance_cpm_minute_data.json:
##########
@@ -0,0 +1,212 @@
+[
+  {
+    "tag_families": [
+      {
+        "tags": [
+          {
+            "id": {
+              "value": "1"
+            }
+          },
+          {
+            "str": {
+              "value": "entity_1"
+            }
+          },
+          {
+            "str": {
+              "value": "svc_1"

Review Comment:
   Could you add an empty service id here to verify the `NULL` group?



##########
banyand/measure/measure_topn.go:
##########
@@ -469,18 +498,20 @@ func (manager *topNProcessorManager) buildMapper(fieldName string, groupByNames
 		return nil, err
 	}
 	return func(_ context.Context, request any) any {
-		dataPoint := request.(*measurev1.DataPointValue)
+		dpWithEvs := request.(*dataPointWithEntityValues)
 		return flow.Data{
+			// EntityValues as identity
+			dpWithEvs.entityValues,
 			// save string representation of group values as the key, i.e. v1
 			strings.Join(transform(groupLocator, func(locator partition.TagLocator) string {
-				return stringify(dataPoint.GetTagFamilies()[locator.FamilyOffset].GetTags()[locator.TagOffset])
+				return stringify(dpWithEvs.GetTagFamilies()[locator.FamilyOffset].GetTags()[locator.TagOffset])
 			}), "|"),
 			// field value as v2
 			// TODO: we only support int64
-			dataPoint.GetFields()[fieldIdx].GetInt().GetValue(),
+			dpWithEvs.GetFields()[fieldIdx].GetInt().GetValue(),
 			// groupBy tag values as v3
 			transform(groupLocator, func(locator partition.TagLocator) *modelv1.TagValue {
-				return dataPoint.GetTagFamilies()[locator.FamilyOffset].GetTags()[locator.TagOffset]
+				return dpWithEvs.GetTagFamilies()[locator.FamilyOffset].GetTags()[locator.TagOffset]

Review Comment:
   If the tag is absent, a panic of `index out of range` might be raised. Suppose there are three tags here, and the group tag is the last one(tag3), [tag1,tag2] whose length is 2 is a valid data point.



##########
test/cases/topn/data/input/asc.yaml:
##########
@@ -16,7 +16,7 @@
 # under the License.
 
 metadata:
-  name: "service_cpm_minute_top_bottom_100"
+  name: "service_instance_cpm_minute_top_bottom_100"
   group: "sw_metric"
 topN: 1

Review Comment:
   Please use a number more than one as we discussed before



##########
banyand/measure/measure_write.go:
##########
@@ -131,9 +131,12 @@ func (s *measure) write(shardID common.ShardID, entity []byte, entityValues tsdb
 	}
 	s.indexWriter.Write(m)
 	if s.processorManager != nil {
-		s.processorManager.onMeasureWrite(&measurev1.WriteRequest{
-			Metadata:  s.GetMetadata(),
-			DataPoint: value,
+		s.processorManager.onMeasureWrite(&measurev1.InternalWriteRequest{
+			Request: &measurev1.WriteRequest{
+				Metadata:  s.GetMetadata(),
+				DataPoint: value,
+			},
+			EntityValues: entityValues[1:],

Review Comment:
   Why do you remove the first value? This value is from the measure's specification. The first is not a reserved entity.



-- 
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