Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 158 additions & 0 deletions astra/src/main/java/com/slack/astra/graphApi/GraphConfig.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
package com.slack.astra.graphApi;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class GraphConfig {
private static final Logger LOG = LoggerFactory.getLogger(GraphConfig.class);

/**
* Represents how a single logical field on a node should be mapped to span tags. Each field has:
* - a default key to look up in tags - a default fallback value if the key isn’t found - an
* optional list of rules that can override the default key Example in YAML: resource:
* default_key: resource default_value: unknown_resource rules: - field: operation_name value:
* http.request override_key: tag.operation.canonical_path In the above example, the default span
* tag for populating a node's resource field is "resource", however if a span's "operation_name"
* == "http.request", then use the "tag.operation.canonical_path" override key to instead populate
* a node's resource field.
*/
public static class TagConfig {
private String defaultKey;
private String defaultValue;
private List<RuleConfig> rules = Collections.emptyList();

public String getDefaultKey() {
return defaultKey;
}

public void setDefaultKey(String defaultKey) {
this.defaultKey = defaultKey;
}

public String getDefaultValue() {
return defaultValue;
}

public void setDefaultValue(String defaultValue) {
this.defaultValue = defaultValue;
}

public List<RuleConfig> getRules() {
return rules;
}

public void setRules(List<RuleConfig> rules) {
this.rules = rules;
}
}

/**
* Represents a conditional rule for overriding which tag key to use. Note: This logic does not
* currently support multiple field matches under a single rule.
*/
public static class RuleConfig {
private String field;
private String value;
private String overrideKey;

public String getField() {
return field;
}

public void setField(String field) {
this.field = field;
}

public String getValue() {
return value;
}

public void setValue(String value) {
this.value = value;
}

public String getOverrideKey() {
return overrideKey;
}

public void setOverrideKey(String overrideKey) {
this.overrideKey = overrideKey;
}
}

// Holds the entire mapping for logical field names to their configuration of defaults and rules.
private Map<String, TagConfig> nodeMetadataTagMapping;

public Map<String, TagConfig> getNodeMetadataTagMapping() {
return nodeMetadataTagMapping;
}

public void setNodeMetadataTagMapping(Map<String, TagConfig> nodeMetadataTagMapping) {
this.nodeMetadataTagMapping = nodeMetadataTagMapping;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!configFile.isEmpty()) {
try {
Path path = Path.of(configFile);
String yaml = Files.readString(path);

ObjectMapper mapper = new ObjectMapper(new YAMLFactory());
mapper.setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE);
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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

return null;
}

/**
* Resolves the actual tag value for a given logical field, using the provided span tags. Steps:
* 1. Look up the TagConfig for this logical field (e.g. "resource"). 2. Default to using its
* defaultKey + defaultValue. 3. If rules are defined: - Iterate through each rule in order. - If
* a rule’s field/value condition matches, switch keyToUse to overrideKey. - Continue processing
* later rules (last matching rule wins). 4. Finally, look up the chosen key in tags. If missing,
* fall back to defaultValue. Note: This logic does not currently support multiple field matches
* for a single rule.
*/
public String resolve(Map<String, String> tags, String logicalField) {
TagConfig baseCfg = nodeMetadataTagMapping.get(logicalField);

// If the config doesn't define this logical field, just return from raw tags or fallback.
if (baseCfg == null) {
return tags.getOrDefault(logicalField, "unknown_" + logicalField);
}

// Start with the default key + value.
String keyToUse = baseCfg.getDefaultKey();
String defaultValue = baseCfg.getDefaultValue();

// 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();
}
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return tags.getOrDefault(keyToUse, defaultValue);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
public class GraphService {
private static final Logger LOG = LoggerFactory.getLogger(GraphService.class);
private final AstraQueryServiceBase searcher;
private final GraphConfig graphConfig;

private static final ObjectMapper objectMapper =
JsonMapper.builder()
Expand All @@ -30,8 +31,11 @@ public class GraphService {
.serializationInclusion(JsonInclude.Include.NON_EMPTY)
.build();

public GraphService(AstraQueryServiceBase searcher) {
public GraphService(AstraQueryServiceBase searcher, GraphConfig graphConfig) {
this.searcher = searcher;
this.graphConfig = graphConfig;

LOG.info("Started GraphService with config: {}", this.graphConfig);
}

@Get
Expand Down
6 changes: 5 additions & 1 deletion astra/src/main/java/com/slack/astra/server/Astra.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.slack.astra.clusterManager.ReplicaRestoreService;
import com.slack.astra.clusterManager.SnapshotDeletionService;
import com.slack.astra.elasticsearchApi.ElasticsearchApiService;
import com.slack.astra.graphApi.GraphConfig;
import com.slack.astra.graphApi.GraphService;
import com.slack.astra.logstore.LogMessage;
import com.slack.astra.logstore.schema.ReservedFields;
Expand Down Expand Up @@ -240,6 +241,9 @@ private static Set<Service> getServices(
// https://github.com/slackhq/astra/pull/564)
final int serverPort = astraConfig.getQueryConfig().getServerConfig().getServerPort();

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ArmeriaService armeriaService =
new ArmeriaService.Builder(serverPort, "astraQuery", meterRegistry)
.withRequestTimeout(requestTimeout)
Expand All @@ -252,7 +256,7 @@ private static Set<Service> getServices(
astraConfig.getQueryConfig().getZipkinDefaultMaxSpans(),
astraConfig.getQueryConfig().getZipkinDefaultLookbackMins(),
astraConfig.getQueryConfig().getZipkinDefaultDataFreshnessSecs()))
.withAnnotatedService(new GraphService(astraDistributedQueryService))
.withAnnotatedService(new GraphService(astraDistributedQueryService, graphConfig))
.withGrpcService(astraDistributedQueryService)
.build();
services.add(armeriaService);
Expand Down
1 change: 1 addition & 0 deletions astra/src/main/proto/astra_configs.proto
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ message QueryServiceConfig {
int32 zipkin_default_max_spans = 4;
int32 zipkin_default_lookback_mins = 5;
int64 zipkin_default_data_freshness_secs = 6;
string dep_graph_config_file = 7;
}

message RedactionUpdateServiceConfig {
Expand Down
Loading
Loading