From ced5c2e60aa22866ba80800ebb5c840cde3cc200 Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Thu, 21 Nov 2024 16:05:49 -0700 Subject: [PATCH 1/7] add depth limiting --- .../com/slack/astra/writer/SpanFormatter.java | 15 ++++++- .../schema/SpanFormatterWithSchemaTest.java | 39 +++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java index d73a519aeb..51126124f6 100644 --- a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java +++ b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java @@ -21,6 +21,7 @@ public class SpanFormatter { public static final String DEFAULT_LOG_MESSAGE_TYPE = "INFO"; public static final String DEFAULT_INDEX_NAME = "unknown"; + public static final int DEPTH_LIMIT = 3; public static Timestamp parseDate(String dateStr, Schema.SchemaFieldType type) { Instant instant; @@ -148,14 +149,24 @@ public static List convertKVtoProto( @VisibleForTesting public static List convertKVtoProtoDefault( String key, Object value, Schema.IngestSchema schema) { + return convertKVtoProtoDefault(key, value, schema, DEPTH_LIMIT - 1); + } + + @VisibleForTesting + public static List convertKVtoProtoDefault( + String key, Object value, Schema.IngestSchema schema, int depthLimit) { List tags = new ArrayList<>(); if (value instanceof Map) { - // todo - consider adding a depth param to prevent excessively nested fields + if (depthLimit <= 0) { + return tags; + } + ((Map) value) .forEach( (key1, value1) -> { List nestedValues = - convertKVtoProtoDefault(String.format("%s.%s", key, key1), value1, schema); + convertKVtoProtoDefault( + String.format("%s.%s", key, key1), value1, schema, depthLimit - 1); tags.addAll(nestedValues); }); } else if (value instanceof String || value instanceof List) { diff --git a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java index a5297c7297..6b91898b28 100644 --- a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java +++ b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java @@ -35,6 +35,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; import java.util.stream.Collectors; import org.apache.commons.io.FileUtils; import org.apache.lucene.document.Document; @@ -157,6 +158,44 @@ public void canParseSimpleMapValues() throws Exception { .isEqualTo("subsubvalue1"); } + @Test + @SuppressWarnings("OptionalGetWithoutIsPresent") + public void stopsAtDepthN() throws Exception { + + Function, Map> nestedMap = + (keyPath) -> { + Object value = "value"; + for (String k : keyPath.reversed()) { + value = Map.of(k, value); + } + return (Map) value; + }; + Map head = nestedMap.apply(List.of("a", "b", "c", "d", "e", "f")); + List list; + list = SpanFormatter.convertKVtoProtoDefault("init", head, schema); + assertThat(list.size()).isEqualTo(0); + + list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 1); + assertThat(list.size()).isEqualTo(0); + + list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 2); + assertThat(list.size()).isEqualTo(0); + + list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 6); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a.b.c.d.e.f"); + + list = + SpanFormatter.convertKVtoProtoDefault("init", nestedMap.apply(List.of("a", "b")), schema); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a.b"); + + list = + SpanFormatter.convertKVtoProtoDefault( + "init", nestedMap.apply(List.of("a", "b", "c")), schema); + assertThat(list.size()).isEqualTo(0); + } + @Test public void parseIndexRequestWithNullValues() throws Exception { final File schemaFile = From 8f211df2680c8d1fd6285965bada17a54537270b Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Fri, 22 Nov 2024 11:15:07 -0700 Subject: [PATCH 2/7] include the field instead of dropping it --- astra/src/main/java/com/slack/astra/writer/SpanFormatter.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java index 51126124f6..63c4c559b9 100644 --- a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java +++ b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java @@ -158,6 +158,9 @@ public static List convertKVtoProtoDefault( List tags = new ArrayList<>(); if (value instanceof Map) { if (depthLimit <= 0) { + // We could encode the value with json instead of toString to make it more friendly to API consumers. + // NB: Using BINARY here to ensure it is not indexed. + tags.add(makeTraceKV(key, value.toString(), Schema.SchemaFieldType.BINARY)); return tags; } From 321fa57ce968dcdda9a38732b49b55d8d6077540 Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Fri, 22 Nov 2024 11:28:05 -0700 Subject: [PATCH 3/7] formatting --- astra/src/main/java/com/slack/astra/writer/SpanFormatter.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java index 63c4c559b9..b2a5c8c2ea 100644 --- a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java +++ b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java @@ -158,7 +158,8 @@ public static List convertKVtoProtoDefault( List tags = new ArrayList<>(); if (value instanceof Map) { if (depthLimit <= 0) { - // We could encode the value with json instead of toString to make it more friendly to API consumers. + // We could encode the value with json instead of toString to make it more friendly to API + // consumers. // NB: Using BINARY here to ensure it is not indexed. tags.add(makeTraceKV(key, value.toString(), Schema.SchemaFieldType.BINARY)); return tags; From 3b218a1fd32f1ef7792f2dc3052ce714b583d86a Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Fri, 22 Nov 2024 11:46:37 -0700 Subject: [PATCH 4/7] fix tests --- .../schema/SpanFormatterWithSchemaTest.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java index 6b91898b28..4847fa5a13 100644 --- a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java +++ b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java @@ -173,13 +173,22 @@ public void stopsAtDepthN() throws Exception { Map head = nestedMap.apply(List.of("a", "b", "c", "d", "e", "f")); List list; list = SpanFormatter.convertKVtoProtoDefault("init", head, schema); - assertThat(list.size()).isEqualTo(0); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a.b"); + assertThat(list.getFirst().getVBinary().toString(StandardCharsets.UTF_8)) + .isEqualTo("{c={d={e={f=value}}}}"); + + list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 0); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init"); list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 1); - assertThat(list.size()).isEqualTo(0); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a"); list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 2); - assertThat(list.size()).isEqualTo(0); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a.b"); list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 6); assertThat(list.size()).isEqualTo(1); @@ -193,7 +202,8 @@ public void stopsAtDepthN() throws Exception { list = SpanFormatter.convertKVtoProtoDefault( "init", nestedMap.apply(List.of("a", "b", "c")), schema); - assertThat(list.size()).isEqualTo(0); + assertThat(list.size()).isEqualTo(1); + assertThat(list.getFirst().getKey()).isEqualTo("init.a.b"); } @Test From 95b9e1983986a13f7af6a2bf5499d18e0c14cd28 Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Fri, 22 Nov 2024 11:47:45 -0700 Subject: [PATCH 5/7] clean up tostring a little --- .../com/slack/astra/schema/SpanFormatterWithSchemaTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java index 4847fa5a13..0d290cba16 100644 --- a/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java +++ b/astra/src/test/java/com/slack/astra/schema/SpanFormatterWithSchemaTest.java @@ -175,8 +175,7 @@ public void stopsAtDepthN() throws Exception { list = SpanFormatter.convertKVtoProtoDefault("init", head, schema); assertThat(list.size()).isEqualTo(1); assertThat(list.getFirst().getKey()).isEqualTo("init.a.b"); - assertThat(list.getFirst().getVBinary().toString(StandardCharsets.UTF_8)) - .isEqualTo("{c={d={e={f=value}}}}"); + assertThat(list.getFirst().getVBinary().toStringUtf8()).isEqualTo("{c={d={e={f=value}}}}"); list = SpanFormatter.convertKVtoProtoDefault("init", head, schema, 0); assertThat(list.size()).isEqualTo(1); From 594fb9a5e532f66b390e753aee9f023c0e260d76 Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Mon, 25 Nov 2024 10:46:42 -0700 Subject: [PATCH 6/7] try depth 1 --- astra/src/main/java/com/slack/astra/writer/SpanFormatter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java index b2a5c8c2ea..f60e4e2dcf 100644 --- a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java +++ b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java @@ -21,7 +21,7 @@ public class SpanFormatter { public static final String DEFAULT_LOG_MESSAGE_TYPE = "INFO"; public static final String DEFAULT_INDEX_NAME = "unknown"; - public static final int DEPTH_LIMIT = 3; + public static final int DEPTH_LIMIT = 1; public static Timestamp parseDate(String dateStr, Schema.SchemaFieldType type) { Instant instant; From d4a471b2aca33511a2ac43349af736743436a28f Mon Sep 17 00:00:00 2001 From: Nora Howard Date: Mon, 25 Nov 2024 11:27:58 -0700 Subject: [PATCH 7/7] move depth limit back to 3 --- astra/src/main/java/com/slack/astra/writer/SpanFormatter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java index f60e4e2dcf..b2a5c8c2ea 100644 --- a/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java +++ b/astra/src/main/java/com/slack/astra/writer/SpanFormatter.java @@ -21,7 +21,7 @@ public class SpanFormatter { public static final String DEFAULT_LOG_MESSAGE_TYPE = "INFO"; public static final String DEFAULT_INDEX_NAME = "unknown"; - public static final int DEPTH_LIMIT = 1; + public static final int DEPTH_LIMIT = 3; public static Timestamp parseDate(String dateStr, Schema.SchemaFieldType type) { Instant instant;