Skip to content

Commit

Permalink
Allow harmonize to return a limited number of nodes (#419)
Browse files Browse the repository at this point in the history
## Description

For our largest graph, running composition yields thousands of build
hints and each build hint can contain hundreds of attached nodes (nodes
are snippets of subgraph SDL that help with locating issues), leading to
very high memory consumption (>10sGB) and can use all of the host
physical memory.

## Investigation

Running composition using the `harmonize` function with dhat enabled; we
can see that most allocations are done when deserializing json from v8
to rust ` serde_v8::de::to_utf8_fast`. On my M1 Mac this was taken right
before the process was killed by the OS and was consuming 45GB of
memory.

![Screenshot 2023-10-16 at 2 17 03
PM](https://github.com/apollographql/federation-rs/assets/1523863/333fdde7-7efb-411e-87a4-102253bf777a)


[dhat-heap-main.json](https://github.com/apollographql/federation-rs/files/12922166/dhat-heap-main.json)


## Fix/Proposal

In the spirit of keeping the library backward compatible, I added a
`harmonize_limit()` function that takes an extra parameter that will
limit the number of nodes returned in each build hint and/or build
error. All nodes above that limit are omitted from the result and a flag
`omitted_nodes_count` is set so the caller is aware some nodes have been
omitted.

If no `nodes_limit` value is passed it defaults to unlimited. The
`harmonize` function defaults to unlimited to preserve backward
compatibility.

Passing `nodes_limit=20` works for us for now, but one could argue there
is limited additional insight past 20 nodes anyway and we could make it
the default instead.
  • Loading branch information
tinnou authored Jan 24, 2024
1 parent f566a5e commit ab24901
Show file tree
Hide file tree
Showing 9 changed files with 171 additions and 25 deletions.
83 changes: 75 additions & 8 deletions apollo-federation-types/src/build/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::{
use serde::{Deserialize, Serialize};

#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
pub struct BuildError {
/// A message describing the build error.
message: Option<String>,
Expand All @@ -21,6 +22,8 @@ pub struct BuildError {
other: crate::UncaughtJson,

nodes: Option<Vec<BuildErrorNode>>,

omitted_nodes_count: Option<u32>,
}

#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
Expand Down Expand Up @@ -87,19 +90,27 @@ impl BuildError {
code: Option<String>,
message: Option<String>,
nodes: Option<Vec<BuildErrorNode>>,
omitted_nodes_count: Option<u32>,
) -> BuildError {
BuildError::new(code, message, BuildErrorType::Composition, nodes)
BuildError::new(
code,
message,
BuildErrorType::Composition,
nodes,
omitted_nodes_count,
)
}

pub fn config_error(code: Option<String>, message: Option<String>) -> BuildError {
BuildError::new(code, message, BuildErrorType::Config, None)
BuildError::new(code, message, BuildErrorType::Config, None, None)
}

fn new(
code: Option<String>,
message: Option<String>,
r#type: BuildErrorType,
nodes: Option<Vec<BuildErrorNode>>,
omitted_nodes_count: Option<u32>,
) -> BuildError {
let real_message = if code.is_none() && message.is_none() {
Some("An unknown error occurred during the build.".to_string())
Expand All @@ -112,6 +123,7 @@ impl BuildError {
r#type,
other: crate::UncaughtJson::new(),
nodes,
omitted_nodes_count,
}
}

Expand All @@ -130,6 +142,10 @@ impl BuildError {
pub fn get_nodes(&self) -> Option<Vec<BuildErrorNode>> {
self.nodes.clone()
}

pub fn get_omitted_nodes_count(&self) -> Option<u32> {
self.omitted_nodes_count
}
}

#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -264,13 +280,19 @@ impl Error for BuildErrors {}
mod tests {
use super::{BuildError, BuildErrors};

use crate::build::BuildErrorNode;
use serde_json::{json, Value};

#[test]
fn it_supports_iter() {
let build_errors: BuildErrors = vec![
BuildError::composition_error(None, Some("wow".to_string()), None),
BuildError::composition_error(Some("BOO".to_string()), Some("boo".to_string()), None),
BuildError::composition_error(None, Some("wow".to_string()), None, None),
BuildError::composition_error(
Some("BOO".to_string()),
Some("boo".to_string()),
None,
None,
),
]
.into();

Expand All @@ -293,9 +315,26 @@ mod tests {

#[test]
fn it_can_serialize_some_build_errors() {
let error_node = BuildErrorNode {
subgraph: Some("foo".to_string()),
source: None,
start: None,
end: None,
};

let build_errors: BuildErrors = vec![
BuildError::composition_error(None, Some("wow".to_string()), None),
BuildError::composition_error(Some("BOO".to_string()), Some("boo".to_string()), None),
BuildError::composition_error(
None,
Some("wow".to_string()),
Some(vec![error_node.clone()]),
Some(1),
),
BuildError::composition_error(
Some("BOO".to_string()),
Some("boo".to_string()),
Some(vec![error_node.clone()]),
Some(2),
),
]
.into();

Expand All @@ -311,16 +350,44 @@ mod tests {
"message": "wow",
"code": null,
"type": "composition",
"nodes": null
"nodes": [
{
"subgraph": "foo",
"source": null,
"start": null,
"end": null
}
],
"omittedNodesCount": 1
},
{
"message": "boo",
"code": "BOO",
"type": "composition",
"nodes": null
"nodes": [
{
"subgraph": "foo",
"source": null,
"start": null,
"end": null
}
],
"omittedNodesCount": 2
}
]
});
assert_eq!(actual_value, expected_value);
}

#[test]
fn it_can_deserialize() {
let msg = "wow".to_string();
let code = "boo".to_string();
let actual_struct = serde_json::from_str(
&json!({ "message": &msg, "code": &code, "type": "composition", "nodes": null, "omittedNodesCount": 12 }).to_string(),
).unwrap();
let expected_struct =
BuildError::composition_error(Some(code.clone()), Some(msg.clone()), None, Some(12));
assert_eq!(expected_struct, actual_struct);
}
}
23 changes: 17 additions & 6 deletions apollo-federation-types/src/build/hint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize};
/// New fields added to this struct must be optional in order to maintain
/// backwards compatibility with old versions of Rover.
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
#[serde(rename_all = "camelCase")]
pub struct BuildHint {
/// The message of the hint
pub message: String,
Expand All @@ -14,17 +15,25 @@ pub struct BuildHint {

pub nodes: Option<Vec<BuildErrorNode>>,

pub omitted_nodes_count: Option<u32>,

/// Other untyped JSON included in the build hint.
#[serde(flatten)]
pub other: crate::UncaughtJson,
}

impl BuildHint {
pub fn new(message: String, code: String, nodes: Option<Vec<BuildErrorNode>>) -> Self {
pub fn new(
message: String,
code: String,
nodes: Option<Vec<BuildErrorNode>>,
omitted_nodes_count: Option<u32>,
) -> Self {
Self {
message,
code: Some(code),
nodes,
omitted_nodes_count,
other: crate::UncaughtJson::new(),
}
}
Expand All @@ -40,8 +49,9 @@ mod tests {
fn it_can_serialize() {
let msg = "hint".to_string();
let code = "hintCode".to_string();
let expected_json = json!({ "message": &msg, "code": &code, "nodes": null });
let actual_json = serde_json::to_value(&BuildHint::new(msg, code, None)).unwrap();
let expected_json =
json!({ "message": &msg, "code": &code, "nodes": null, "omittedNodesCount": null });
let actual_json = serde_json::to_value(&BuildHint::new(msg, code, None, None)).unwrap();
assert_eq!(expected_json, actual_json)
}

Expand All @@ -50,10 +60,11 @@ mod tests {
let msg = "hint".to_string();
let code = "hintCode".to_string();
let actual_struct = serde_json::from_str(
&json!({ "message": &msg, "code": &code, "nodes": null }).to_string(),
&json!({ "message": &msg, "code": &code, "nodes": null, "omittedNodesCount": 12 })
.to_string(),
)
.unwrap();
let expected_struct = BuildHint::new(msg, code, None);
let expected_struct = BuildHint::new(msg, code, None, Some(12));
assert_eq!(expected_struct, actual_struct);
}

Expand All @@ -68,7 +79,7 @@ mod tests {
.to_string(),
)
.unwrap();
let mut expected_struct = BuildHint::new(msg, code, None);
let mut expected_struct = BuildHint::new(msg, code, None, None);
expected_struct
.other
.insert(unexpected_key, Value::String(unexpected_value));
Expand Down
10 changes: 5 additions & 5 deletions apollo-federation-types/src/build/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ mod tests {
let hint_two = "hint-two".to_string();
let code = "code".to_string();
let code2 = "code2".to_string();
let expected_json = json!({"supergraphSdl": &sdl, "hints": [{"message": &hint_one, "code": &code, "nodes": null}, {"message": &hint_two, "code": &code2, "nodes": null}]});
let expected_json = json!({"supergraphSdl": &sdl, "hints": [{"message": &hint_one, "code": &code, "nodes": null, "omittedNodesCount": null}, {"message": &hint_two, "code": &code2, "nodes": null, "omittedNodesCount": null}]});
let actual_json = serde_json::to_value(&BuildOutput::new_with_hints(
sdl.to_string(),
vec![
BuildHint::new(hint_one, code, None),
BuildHint::new(hint_two, code2, None),
BuildHint::new(hint_one, code, None, None),
BuildHint::new(hint_two, code2, None, None),
],
))
.unwrap();
Expand Down Expand Up @@ -91,8 +91,8 @@ mod tests {
let expected_struct = BuildOutput::new_with_hints(
sdl,
vec![
BuildHint::new(hint_one, code, None),
BuildHint::new(hint_two, code2, None),
BuildHint::new(hint_one, code, None, None),
BuildHint::new(hint_two, code2, None, None),
],
);

Expand Down
2 changes: 1 addition & 1 deletion federation-1/harmonizer/src/js_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl Display for CompositionError {

impl From<CompositionError> for BuildError {
fn from(input: CompositionError) -> Self {
Self::composition_error(input.code, input.message, input.nodes)
Self::composition_error(input.code, input.message, input.nodes, None)
}
}

Expand Down
36 changes: 33 additions & 3 deletions federation-2/harmonizer/js-src/composition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ import {
} from "./types";
import { ERRORS } from "@apollo/federation-internals";

const DEFAULT_NODES_SIZE_LIMIT: number = Number.MAX_VALUE;

export function composition(
serviceList: { sdl: string; name: string; url?: string }[]
serviceList: { sdl: string; name: string; url?: string }[],
nodesLimit?: number | null
): CompositionResult {
let limit = nodesLimit || DEFAULT_NODES_SIZE_LIMIT;
if (!serviceList || !Array.isArray(serviceList)) {
throw new Error("Error in JS-Rust-land: serviceList missing or incorrect.");
}
Expand All @@ -38,12 +42,25 @@ export function composition(
if (composed.hints) {
composed.hints.map((composed_hint) => {
let nodes: BuildErrorNode[] = [];
composed_hint.nodes?.map((node) => nodes.push(getBuildErrorNode(node)));

let omittedNodesCount = 0;
// for issues that happen in all subgraphs and with a large amount of subgraphs,
// only add nodes up to the limit to prevent massive responses
// (OOM errors when going from js to rust)
if (composed_hint.nodes?.length >= limit) {
composed_hint.nodes
?.slice(0, limit)
.map((node) => nodes.push(getBuildErrorNode(node)));
omittedNodesCount = composed_hint.nodes?.length - limit;
} else {
composed_hint.nodes?.map((node) => nodes.push(getBuildErrorNode(node)));
}

hints.push({
message: composed_hint.toString(),
code: composed_hint.definition.code,
nodes,
omittedNodesCount: omittedNodesCount,
});
});
}
Expand All @@ -53,12 +70,25 @@ export function composition(
let errors: CompositionError[] = [];
composed.errors.map((err) => {
let nodes: BuildErrorNode[] = [];
err.nodes?.map((node) => nodes.push(getBuildErrorNode(node)));

let omittedNodesCount = 0;
// for issues that happen in all subgraphs and with a large amount of subgraphs,
// only add nodes up to the limit to prevent massive responses
// (OOM errors when going from js to rust)
if (err.nodes?.length >= limit) {
err.nodes
?.slice(0, limit)
.map((node) => nodes.push(getBuildErrorNode(node)));
omittedNodesCount = err.nodes?.length - limit;
} else {
err.nodes?.map((node) => nodes.push(getBuildErrorNode(node)));
}

errors.push({
code: (err?.extensions["code"] as string) ?? "",
message: err.message,
nodes,
omittedNodesCount: omittedNodesCount,
});
});

Expand Down
3 changes: 2 additions & 1 deletion federation-2/harmonizer/js-src/do_compose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@ declare let composition_bridge: { composition: typeof composition };

declare let done: (compositionResult: CompositionResult) => void;
declare let serviceList: { sdl: string; name: string; url?: string }[];
declare let nodesLimit: number | null;

try {
// /**
// * @type {{ errors: Error[], supergraphSdl?: undefined, hints: undefined } | { errors?: undefined, supergraphSdl: string, hints: string }}
// */
const composed = composition_bridge.composition(serviceList);
const composed = composition_bridge.composition(serviceList, nodesLimit);

done(composed);
} catch (err) {
Expand Down
2 changes: 2 additions & 0 deletions federation-2/harmonizer/js-src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export type CompositionError = {
message?: string;
code?: string;
nodes: BuildErrorNode[];
omittedNodesCount: number;
};

export type BuildErrorNode = {
Expand All @@ -26,4 +27,5 @@ export type CompositionHint = {
message: string;
code: string;
nodes: BuildErrorNode[];
omittedNodesCount: number;
};
Loading

0 comments on commit ab24901

Please sign in to comment.