fix(reduce transform): use the correct merge strategy for top level objects #21067

pront · 2024-08-13T19:34:12Z

This fix addresses a regression introduced by PR #20800.

The add_event function was modified to workaround the fact that the event iterator returns paths in a DFS fashion (and also it doesn't return the top level field).

As a workaround:

it now iterates over the configured strategies and instantiates the proper value merger
if a path is covered by the above, then it is ignored (no value merger)

Another alternative considered here is a BFS event path traversal but that seemed like a more time consuming change. Also, arguably, the event iterators should return a OwnedTargetPath instead of a KeyString.

datadog-vectordotdev · 2024-08-13T20:10:28Z

Datadog Report

Branch report: pront/21065-fix
Commit report: 338d684
Test service: vector

✅ 0 Failed, 7 Passed, 0 Skipped, 25.49s Total Time

…gies for both 'a' and 'a.b'

jszwedko · 2024-08-14T20:06:58Z

It looks like this fails to run the example test case in #21065 still. I see:

2024-08-14T20:05:32.608574Z ERROR transform{component_kind="transform" component_id=reduce_scalyr component_type=reduce}: vector::transforms::reduce::transform: error=Invalid field path "sessionInfo.kubernetes\\.cluster" rate_limit=30
2024-08-14T20:05:32.608732Z ERROR transform{component_kind="transform" component_id=reduce_scalyr component_type=reduce}: vector::transforms::reduce::transform: error=Invalid field path "sessionInfo.kubernetes\\.cluster" rate_limit=30
test map_reduce_scalyr.no_sev ... failed

changelog.d/21067_reduce.fix.md

jszwedko

The general approach here seems sound to me. Just a couple of comments and what seems like an issue with path parsing 🤔

src/transforms/reduce/transform.rs

jszwedko · 2024-08-15T15:17:14Z

src/transforms/reduce/transform.rs

+        for (path, strategy) in strategies {
+            if let Some(value) = e.get(path) {
+                match self.fields.entry(path.clone()) {
+                    Entry::Vacant(entry) => match get_value_merger(value.clone(), strategy) {
+                        Ok(m) => {
+                            entry.insert(m);
+                        }
+                        Err(error) => {
+                            warn!(message = "Failed to create value merger.", %error, %path);
+                        }
+                    },
+                    Entry::Occupied(mut entry) => {
+                        if let Err(error) = entry.get_mut().add(value.clone()) {
+                            warn!(message = "Failed to merge value.", %error);
+                        }
+                    }
+                }
+            }
+        }


I see the warnings here follow the existing pattern for get_value_merger failures, but I am wondering if these shouldn't be emitting events that have metrics too 🤔

I'm ok leaving this for a follow-up since it doesn't seem to make the situation worse.

jszwedko

Nice! I verified that the example from the bug report passes now.

src/transforms/reduce/transform.rs

src/internal_events/reduce.rs

src/transforms/reduce/transform.rs

jszwedko

Thanks for addressing the final points!

github-actions · 2024-08-16T20:25:42Z

Regression Detector Results

Run ID: 47059084-5343-43f9-b92e-4b789f7b2a1c Metrics dashboard

Baseline: ac4e194
Comparison: f7e4470

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI	links
❌	file_to_blackhole	egress throughput	-16.84	[-23.14, -10.54]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	datadog_agent_remap_datadog_logs	ingress throughput	+3.17	[+2.99, +3.35]
➖	syslog_regex_logs2metric_ddmetrics	ingress throughput	+2.14	[+1.95, +2.33]
➖	datadog_agent_remap_datadog_logs_acks	ingress throughput	+1.80	[+1.59, +2.00]
➖	syslog_humio_logs	ingress throughput	+1.45	[+1.34, +1.55]
➖	syslog_splunk_hec_logs	ingress throughput	+1.42	[+1.33, +1.51]
➖	syslog_log2metric_splunk_hec_metrics	ingress throughput	+1.40	[+1.28, +1.52]
➖	http_elasticsearch	ingress throughput	+1.13	[+0.97, +1.30]
➖	fluent_elasticsearch	ingress throughput	+1.01	[+0.51, +1.51]
➖	otlp_http_to_blackhole	ingress throughput	+0.89	[+0.76, +1.02]
➖	syslog_loki	ingress throughput	+0.77	[+0.63, +0.90]
➖	datadog_agent_remap_blackhole_acks	ingress throughput	+0.74	[+0.63, +0.86]
➖	syslog_log2metric_tag_cardinality_limit_blackhole	ingress throughput	+0.59	[+0.50, +0.67]
➖	http_to_http_noack	ingress throughput	+0.18	[+0.09, +0.27]
➖	http_to_s3	ingress throughput	+0.14	[-0.12, +0.41]
➖	splunk_hec_route_s3	ingress throughput	+0.10	[-0.21, +0.42]
➖	http_to_http_json	ingress throughput	+0.09	[+0.03, +0.15]
➖	splunk_hec_to_splunk_hec_logs_noack	ingress throughput	+0.03	[-0.07, +0.13]
➖	splunk_hec_to_splunk_hec_logs_acks	ingress throughput	+0.00	[-0.11, +0.12]
➖	splunk_hec_indexer_ack_blackhole	ingress throughput	-0.00	[-0.08, +0.08]
➖	datadog_agent_remap_blackhole	ingress throughput	-0.05	[-0.17, +0.07]
➖	otlp_grpc_to_blackhole	ingress throughput	-0.06	[-0.18, +0.05]
➖	http_to_http_acks	ingress throughput	-0.11	[-1.43, +1.20]
➖	syslog_log2metric_humio_metrics	ingress throughput	-0.83	[-0.96, -0.70]
➖	http_text_to_http_json	ingress throughput	-0.96	[-1.10, -0.83]
➖	socket_to_socket_blackhole	ingress throughput	-1.51	[-1.57, -1.46]
❌	file_to_blackhole	egress throughput	-16.84	[-23.14, -10.54]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

…bjects (#21067) * fix(issue 21065): use the correct merge strategy for top level objects * changelog * revert config changes to preserve backwards compatibility * enhance test case * update test with another merge strategy, note we cannot handle strategies for both 'a' and 'a.b' * tweak changelog text * workaround for issue 21077 * add nested path to the test case * Address review points * add dot to message * more review points

…bjects (vectordotdev#21067) * fix(issue 21065): use the correct merge strategy for top level objects * changelog * revert config changes to preserve backwards compatibility * enhance test case * update test with another merge strategy, note we cannot handle strategies for both 'a' and 'a.b' * tweak changelog text * workaround for issue 21077 * add nested path to the test case * Address review points * add dot to message * more review points

fix(issue 21065): use the correct merge strategy for top level objects

7546afc

github-actions bot added the domain: transforms Anything related to Vector's transform components label Aug 13, 2024

changelog

7158ca9

pront marked this pull request as ready for review August 13, 2024 19:49

pront requested a review from a team as a code owner August 13, 2024 19:49

pront marked this pull request as draft August 13, 2024 19:51

revert config changes to preserve backwards compatibility

8e8c32b

pront force-pushed the pront/21065-fix branch from 12a727b to 8e8c32b Compare August 13, 2024 20:10

enhance test case

5036760

pront marked this pull request as ready for review August 13, 2024 20:15

pront requested a review from jszwedko August 13, 2024 20:15

update test with another merge strategy, note we cannot handle strate…

7eefa1a

…gies for both 'a' and 'a.b'

jszwedko added this to the Vector v0.40.1 milestone Aug 14, 2024

jszwedko reviewed Aug 14, 2024

View reviewed changes

changelog.d/21067_reduce.fix.md Outdated Show resolved Hide resolved

jszwedko changed the title ~~fix(issue 21065): use the correct merge strategy for top level objects~~ fix(reduce transform): use the correct merge strategy for top level objects Aug 14, 2024

jszwedko reviewed Aug 14, 2024

View reviewed changes

src/transforms/reduce/transform.rs Outdated Show resolved Hide resolved

pront added 3 commits August 14, 2024 16:58

tweak changelog text

63e5cce

workaround for issue 21077

d17b7a5

add nested path to the test case

a857bac

jszwedko reviewed Aug 15, 2024

View reviewed changes

Address review points

c7c07ae

pront force-pushed the pront/21065-fix branch from 6bbab4d to c7c07ae Compare August 16, 2024 15:13

add dot to message

04b181b

pront requested a review from jszwedko August 16, 2024 18:06

jszwedko reviewed Aug 16, 2024

View reviewed changes

src/transforms/reduce/transform.rs Outdated Show resolved Hide resolved

src/internal_events/reduce.rs Outdated Show resolved Hide resolved

src/internal_events/reduce.rs Show resolved Hide resolved

src/transforms/reduce/transform.rs Outdated Show resolved Hide resolved

more review points

f32a2b7

pront requested a review from jszwedko August 16, 2024 19:13

jszwedko approved these changes Aug 16, 2024

View reviewed changes

pront added this pull request to the merge queue Aug 16, 2024

Merged via the queue into master with commit f7e4470 Aug 16, 2024
81 checks passed

pront deleted the pront/21065-fix branch August 16, 2024 20:43

pront mentioned this pull request Sep 3, 2024

fix(reduce transform): surround invalid path segments with quotes #21201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(reduce transform): use the correct merge strategy for top level objects #21067

fix(reduce transform): use the correct merge strategy for top level objects #21067

pront commented Aug 13, 2024 •

edited

Loading

datadog-vectordotdev bot commented Aug 13, 2024 •

edited

Loading

jszwedko commented Aug 14, 2024

jszwedko left a comment

jszwedko Aug 15, 2024

jszwedko left a comment

jszwedko left a comment

github-actions bot commented Aug 16, 2024

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

fix(reduce transform): use the correct merge strategy for top level objects #21067

fix(reduce transform): use the correct merge strategy for top level objects #21067

Conversation

pront commented Aug 13, 2024 • edited Loading

datadog-vectordotdev bot commented Aug 13, 2024 • edited Loading

Datadog Report

jszwedko commented Aug 14, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko Aug 15, 2024

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2024

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

pront commented Aug 13, 2024 •

edited

Loading

datadog-vectordotdev bot commented Aug 13, 2024 •

edited

Loading