dep graph config loader #52

pbibra · 2025-09-05T23:45:37Z

Summary

As per: #51 (comment), we'd like to abstract away fork specific span schema details when building nodes for a trace dependency graph.

This PR introduces a configurable dependency graph node metadata mapping for Zipkin spans. Instead of hardcoding tag names for filling in node metadata, the logic can be driven by a YAML configuration file. This makes field resolution more flexible and maintainable, while allowing conditional overrides for specific span types.

The config format is defined as follow:

node_metadata_tag_mapping:
  <metadata-field-name>:
    default_key: <span.tag>
    default_value: <default-value>
    rules:
      - field: <span.tag>
        value: <match-value>
        override_key: <span.tag>

where rules can be a list on various fields which will be applied sequentially

The GraphConfig class:

Supports node_metadata_tag_mapping for defining default tag keys and fallback values.
Supports rules, grouped by logical field, to conditionally override which tag key to use based on other span attributes.
Exposes a resolve(...) method to compute the correct tag value for a given field, taking into account both defaults and applicable rules.

The GraphService will now take in a GraphConfig instance. If the config file is not provided at startup, this instance will be null in which case, the eventual graph builder should fall back to reasonable defaults e.g. using required span fields as metadata.

Testing

 ▲ ~ kubectl logs --context=m-obs-b-ea1-us --namespace=airtrace-traces-staging airtrace-query-traces-staging-59f75986c-pspsz | grep INFO
{"@timestamp":"2025-09-12T21:18:35.567Z","level":"INFO","message":"Using the APIs optimized for: Java 12+","thread.name":"armeria-common-worker-epoll-3-2","logger_name":"com.linecorp.armeria.internal.common.JavaVersionSpecific"}
{"@timestamp":"2025-09-12T21:18:57.676Z","level":"INFO","message":"GraphConfig: {\n  \"nodeMetadataTagMapping\" : {\n    \"app\" : {\n      \"defaultKey\" : \"kube.app\",\n      \"defaultValue\" : \"unknown_app\",\n      \"rules\" : [ ]\n    },\n    \"namespace\" : {\n      \"defaultKey\" : \"kube-namespace\",\n      \"defaultValue\" : \"unknown_namespace\",\n      \"rules\" : [ ]\n    },\n    \"operation\" : {\n      \"defaultKey\" : \"operation_name\",\n      \"defaultValue\" : \"unknown_operation\",\n      \"rules\" : [ ]\n    },\n    \"resource\" : {\n      \"defaultKey\" : \"resource\",\n      \"defaultValue\" : \"unknown_resource\",\n      \"rules\" : [ {\n        \"field\" : \"operation_name\",\n        \"value\" : \"http.request\",\n        \"overrideKey\" : \"tag.operation.canonical_path\"\n      } ]\n    }\n  }\n}","thread.name":"armeria-common-worker-epoll-3-3","logger_name":"com.slack.astra.graphApi.GraphService","trace_id":"b096b3282ceca4c0","span_id":"b096b3282ceca4c0"}

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

baroquebobcat · 2025-09-16T21:46:04Z

astra/src/main/java/com/slack/astra/graphApi/GraphConfig.java

+    // Apply rules in order. Later matching rules override earlier ones.
+    for (RuleConfig rule : baseCfg.getRules()) {
+      // Get the value of the field defined in the rule.
+      String matchVal = tags.getOrDefault(rule.getField(), "unknown_" + rule.getField());
+      // If the rule value matches, override which key to use for resolution of the logical field
+      // only if it exists in tags, otherwise keep the default.
+      if (rule.getValue().equals(matchVal)) {
+        if (tags.containsKey(rule.getOverrideKey())) {
+          keyToUse = rule.getOverrideKey();
+        }
+      }
+    }


given the later matching override, I'd probably implement this by reversing the list and picking the first one that matches. Using streams, it could look like this.

Suggested change

// Apply rules in order. Later matching rules override earlier ones.

for (RuleConfig rule : baseCfg.getRules()) {

// Get the value of the field defined in the rule.

String matchVal = tags.getOrDefault(rule.getField(), "unknown_" + rule.getField());

// If the rule value matches, override which key to use for resolution of the logical field

// only if it exists in tags, otherwise keep the default.

if (rule.getValue().equals(matchVal)) {

if (tags.containsKey(rule.getOverrideKey())) {

keyToUse = rule.getOverrideKey();

}

}

}

// Later rules override earlier ones, so start from the back of the list and use the first one that matches

// where the rule's override key exists in the document.

String keyToUse = baseCfg.getRules().reversed().stream()

.filter(rule -> rule.getValue().equals(tags.getOrDefault(rule.getField(), "unknown_" + rule.getField()))

.map(RuleConfig::getOverrideKey)

.filter(tags::containsKey)

.findFirst().orElse(baseCfg.getDefaultKey());

having a method on Rule for checking the field would make that read cleaner by making the first filter less noisy.

Also if the resolve happens in a hot path, you might want to do the reversing of the list at config construction time, among other things, but that should probably happen later.

baroquebobcat · 2025-09-16T21:50:12Z

astra/src/test/java/com/slack/astra/graphApi/GraphConfigTest.java

+public class GraphConfigTest {
+
+  @Test
+  public void testTagConfigGettersAndSetters() {


Could you leave off the getter / setter testing? I think the main behavior we're gonna care about is how resolution/normalization works, so I'd expect most of the tests to be about that.

Also, I would expect that the caller wouldn't care about the specifics of the default key/values.

baroquebobcat · 2025-09-16T21:58:57Z

astra/src/main/java/com/slack/astra/server/Astra.java

+      GraphConfig graphConfig =
+          GraphConfig.load(astraConfig.getQueryConfig().getDepGraphConfigFile());
+


I'd prefer if this bit followed the pattern of the schema file, where if the file is not defined, we log that it isn't and use the default whatever that ends up being.

https://github.com/airbnb/kaldb/pull/52/files#diff-1def4f01c8d94dbbb54e4c9ed39002029aa0861c11d07f0d38d660973b80c815R519-R528

baroquebobcat · 2025-09-16T21:59:36Z

astra/src/main/java/com/slack/astra/graphApi/GraphConfig.java

+        return mapper.readValue(yaml, GraphConfig.class);
+      } catch (Exception e) {
+        LOG.warn("Failed to read or parse dependency graph config file. Returning null config", e);
+        return null;


Instead of null, could this return a default instance that maybe noops resolution or something?

baroquebobcat · 2025-09-16T22:06:03Z

astra/src/test/java/com/slack/astra/graphApi/GraphConfigTest.java

+    GraphConfig.TagConfig serviceConfig = config.getNodeMetadataTagMapping().get("service");
+    assertThat(serviceConfig.getDefaultKey()).isEqualTo("service.name");
+    assertThat(serviceConfig.getDefaultValue()).isEqualTo("unknown_service");
+    assertThat(serviceConfig.getRules()).hasSize(2);
+
+    GraphConfig.RuleConfig rule1 = serviceConfig.getRules().getFirst();
+    assertThat(rule1.getOverrideKey()).isEqualTo("prod.service.name");
+    assertThat(rule1.getField()).isEqualTo("cluster.name");
+    assertThat(rule1.getValue()).isEqualTo("prod");
+
+    GraphConfig.RuleConfig rule2 = serviceConfig.getRules().get(1);
+    assertThat(rule2.getOverrideKey()).isEqualTo("test.service.name");
+    assertThat(rule2.getField()).isEqualTo("cluster.name");
+    assertThat(rule2.getValue()).isEqualTo("staging");
+
+    GraphConfig.TagConfig clusterConfig = config.getNodeMetadataTagMapping().get("cluster");
+    assertThat(clusterConfig.getDefaultKey()).isEqualTo("cluster.name");
+    assertThat(clusterConfig.getDefaultValue()).isEqualTo("unknown_cluster");
+    assertThat(clusterConfig.getRules()).hasSize(0);


how about instead of these, do something more like this, covering the rules and making sure to catch the fallthrough behavior.

var emptyTestMap = Map.of(); assertThat(config.resolve(emptyTestMap, "service").isEqualTo("unknown_service"); // ... etc etc var testMap = Map.of("service.name", "something", "cluster.name", "prod"); assertThat(config.resolve(testMap, "service").isEqualTo("something"); assertThat(config.resolve(testMap, "cluster").isEqualTo("prod....");

baroquebobcat · 2025-09-16T22:07:22Z

astra/src/test/java/com/slack/astra/graphApi/GraphConfigTest.java

+  }
+
+  @Test
+  public void testResolveWithDefaultValue() {


Oh, I see, you have some of the resolution tests later.

baroquebobcat · 2025-09-16T22:10:10Z

astra/src/main/java/com/slack/astra/graphApi/GraphConfig.java

+  }
+
+  // Loads a GraphConfig from a YAML file on disk. On failure, log and return a null config.
+  public static GraphConfig load(String configFile) throws IOException {


How about breaking this up into a load(Path filePath) and load(String config)? that would make it easier to test against.

baroquebobcat · 2025-09-16T22:11:45Z

astra/src/test/java/com/slack/astra/graphApi/GraphConfigTest.java

+
+  @Test
+  public void testResolveWithDefaultValue() {
+    GraphConfig config = createTestConfig();


This way of referring to a test fixture makes the test harder to follow. If you had GraphConfig.load(String configContents), you could have each test do GraphConfig.load("""<yaml>""");, which would make them easier to skim.

baroquebobcat · 2025-09-16T22:17:48Z

astra/src/test/java/com/slack/astra/graphApi/GraphConfigTest.java

+        """
+        nodeMetadataTagMapping:
+          service:
+            defaultKey: service.name
+            defaultValue: unknown_service
+            rules:
+              - field: cluster.name
+                value: prod
+                overrideKey: prod.service.name
+              - field: cluster.name
+                value: staging
+                overrideKey: test.service.name
+          cluster:
+            defaultKey: cluster.name
+            defaultValue: unknown_cluster


having the yaml here in the test case is nice

Priya Bibra and others added 18 commits September 5, 2025 23:34

initial graph config ingest implementation

15a7dc9

Merge branch 'airbnb-main' into pbibra-graph-config

5dc3f6e

revert pom

bcddc1c

format

a69649b

fix proto

db6b7b3

fix var name

fdb7fab

add unit tests

f329b36

format

a557973

merge airbnb-main and fix conflicts

04870f3

remove graphconfig from zipkin service

ff070a2

update logic and tests

152a58c

send config to GraphService

402fa40

add to string + log

a401dc8

change loctaion of log

2dc8439

debug

dd8d1fb

update object mapper for snake case

6065cff

cleanup

4b1bee8

more cleanup

82a6bfc

pbibra marked this pull request as ready for review September 12, 2025 21:36

pbibra requested a review from baroquebobcat September 12, 2025 21:36

baroquebobcat reviewed Sep 16, 2025

View reviewed changes

Priya Bibra added 6 commits September 17, 2025 16:48

make graphconfig final, refactor load and resolve

5e6ec59

log debug

4d02110

re-add tostring

5919cee

cleanup

497e564

revert log line in curatorbuilder

7b19cae

reformat comments

5515097

baroquebobcat approved these changes Sep 19, 2025

View reviewed changes

pbibra merged commit f825cb2 into airbnb-main Sep 19, 2025
2 checks passed

pbibra deleted the pbibra-graph-config branch September 19, 2025 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

dep graph config loader #52

dep graph config loader #52

Uh oh!

pbibra commented Sep 5, 2025 •

edited

Loading

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

baroquebobcat Sep 16, 2025

Uh oh!

Uh oh!

Uh oh!

		GraphConfig graphConfig =
		GraphConfig.load(astraConfig.getQueryConfig().getDepGraphConfigFile());

dep graph config loader #52

dep graph config loader #52

Uh oh!

Conversation

pbibra commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Requirements

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pbibra commented Sep 5, 2025 •

edited

Loading