Support Nested matching via JsonPathValue/logFieldJsonPath - #572
Support Nested matching via JsonPathValue/logFieldJsonPath#572FranAguilera wants to merge 1 commit into
Conversation
4cf0502 to
0f9ae96
Compare
FranAguilera
changed the title
f73c2b3 to
bc1e8fe
Compare
bc1e8fe to
82565e4
Compare
| // Existing SDK APIs send JSON as a string. Parsing is only reached from JsonPathValue matchers. | ||
| // TODO: Gate JSON string extraction with a remote workflow runtime flag. | ||
| if let Some(json) = value.as_str() { | ||
| return resolve_json_string_path(json, path); |
There was a problem hiding this comment.
Pending to add a killswitch flag here so we can disable remotely in case something is off
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
I think we need a depth bound on the parser to deal with bad input but other than that all my comments are just suggestions or style guidance, good job!
| escaped: bool, | ||
| } | ||
|
|
||
| enum PathResult<'a> { |
There was a problem hiding this comment.
I think this is just Option<Cow<'a, str>>, I don't think the type adds that much and prevents ? usage
Looking at how we do return Option<PathResult<_>> I'd either model it as
enum PathResult {
Found(Cow<'a, str>,
Missing,
Invalid
}
if we want to understand the tri-state or just collapse it to an Option<Cow<'a, str>> for simplicity
| } | ||
|
|
||
| #[test] | ||
| #[ignore = "run manually with --release -- --ignored --nocapture"] |
There was a problem hiding this comment.
This would be better off as a criterion test - I just added one that we can use for reference. It handles the standard stuff like running it multiple times, percentiles etc. Not a blocker for this PR but it should make the benchmarking experience better
| Self { input, pos: 0 } | ||
| } | ||
|
|
||
| fn walk_value(&mut self, path: &[JsonPathToken]) -> Option<PathResult<'a>> { |
There was a problem hiding this comment.
With these kind of recursive parsers one thing we have to be careful about when we don't control the input is stack overflow. The simplest thing to do is to add a depth limit (say 128) to prevent going to deep, or switch over to an iterative approach that doesn't have the same per-depth overhead. I think a depth limit is reasonable so we can probably start there unless we believe someone really cares about super complex JSON objects
There was a problem hiding this comment.
great idea, will add
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Per the guidelines these would go in json_path_test.rs, try out https://github.com/bitdriftlabs/ai-instructions if you need to set up your agent and it should do this automatically
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
I'd probably add some more tests for correctness, there's a lot of test surface. The agent will do a good job here
| } | ||
|
|
||
| impl<'a> Arbitrary<'a> for JsonPathFuzzTestCase { | ||
| fn arbitrary(input: &mut Unstructured<'a>) -> arbitrary::Result<Self> { |
There was a problem hiding this comment.
One thing you can do here to validate correctness via fuzzing is to separately implement this via serde_json - the implementation is much simpler and you get to validate that our more complex custom parser agrees with the simple implementation. Then you can let it run and catch any issues (or have the agent do it)
Love using a fuzzer for this!
| fn resolve_json_path<'a>(value: &'a DataValue, path: &[JsonPathToken]) -> Option<Cow<'a, str>> { | ||
| // Existing SDK APIs send JSON as a string. Parsing is only reached from JsonPathValue matchers. | ||
| // TODO: Gate JSON string extraction with a remote workflow runtime flag. | ||
| if let Some(json) = value.as_str() { |
There was a problem hiding this comment.
just for code clarity I would do something like if let Some(maybe_json) ... as I think it more clearly captures that we aren't expecting JSON here but are opportunistically trying to extract a JSON value per the matcher
| } | ||
|
|
||
| impl<'a> Arbitrary<'a> for JsonPathFuzzTestCase { | ||
| fn arbitrary(input: &mut Unstructured<'a>) -> arbitrary::Result<Self> { |
There was a problem hiding this comment.
I don't know if serde_json supports arbitrary out of the box but it would be interesting to have the agent 1) generate sensible JSON and 2) try to ensure that occasionally they match. If you look at the state fuzzer we do stuff like that to try to guide it to sane values
| } | ||
| } | ||
|
|
||
| struct Parser<'a> { |
There was a problem hiding this comment.
I would add some docs on the perf implications of this, there's a few different things worth calling out when it needs to allocate vs not e.g. for escaped values
2 participants
Adds support for nested json matching via the existing
Leaf::JsonPathValue, for this initial prototype I'm proposing to use existing JSON string to extract the nested conditionsMore details, regarding frotnend/rollout/performance etc in the internal PRD here
You can run local benchmark tests with
make benchmark-json-matcherThis is used in bitdriftlabs/capture-sdk#1079
NOTE: Pending to flag new logic for json string