[pkg/ottl] Handle nil val in IsMatch #17572

TylerHelmuth · 2023-01-13T16:25:03Z

Description:
Fixes a bug where IsMatch would return an error if the target returned a nil val. This broke conditions that used IsMatch as it is reasonable to check an attribute in a where clause that might not exist.

Link to tracking Issue:
Fixes #17563

Testing:
Added new unit test

runforesight · 2023-01-13T16:26:22Z

Foresight Summary

Major Impacts

build-and-test-windows duration(3 seconds) has decreased 42 minutes 29 seconds compared to main branch avg(42 minutes 32 seconds).

View More Details

✅ check-links workflow has finished in 46 seconds (1 minute 14 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
changed files	- 🔗	N/A	See Details
check-links	- 🔗	N/A	See Details

✅ tracegen workflow has finished in 56 seconds (1 minute 36 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
build-dev	- 🔗	N/A	See Details
publish-latest	- 🔗	N/A	See Details
publish-stable	- 🔗	N/A	See Details

✅ telemetrygen workflow has finished in 58 seconds (1 minute 39 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
build-dev	- 🔗	N/A	See Details
publish-latest	- 🔗	N/A	See Details
publish-stable	- 🔗	N/A	See Details

✅ prometheus-compliance-tests workflow has finished in 3 minutes 43 seconds (4 minutes 12 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
prometheus-compliance-tests	- 🔗	✅ 21 ❌ 0 ⏭ 0 🔗	See Details

✅ load-tests workflow has finished in 8 minutes 56 seconds (6 minutes 6 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
loadtest (TestIdleMode)	- 🔗	✅ 1 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestTraceAttributesProcessor)	- 🔗	✅ 3 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestMetric10kDPS\|TestMetricsFromFile)	- 🔗	✅ 6 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestTraceNoBackend10kSPS\|TestTrace1kSPSWithAttrs)	- 🔗	✅ 8 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestMetricResourceProcessor\|TestTrace10kSPS)	- 🔗	✅ 12 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestTraceBallast1kSPSWithAttrs\|TestTraceBallast1kSPSAddAttrs)	- 🔗	✅ 10 ❌ 0 ⏭ 0 🔗	See Details
loadtest (TestBallastMemory\|TestLog10kDPS)	- 🔗	✅ 19 ❌ 0 ⏭ 0 🔗	See Details
setup-environment	- 🔗	N/A	See Details

✅ build-and-test workflow has finished in 40 minutes 11 seconds (8 minutes 44 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

Job	Failed Steps	Tests
integration-tests	- 🔗	✅ 55 ❌ 0 ⏭ 0 🔗	See Details
cross-compile (darwin, amd64)	- 🔗	N/A	See Details
cross-compile (darwin, arm64)	- 🔗	N/A	See Details
cross-compile (linux, amd64)	- 🔗	N/A	See Details
cross-compile (linux, 386)	- 🔗	N/A	See Details
cross-compile (linux, arm)	- 🔗	N/A	See Details
cross-compile (linux, arm64)	- 🔗	N/A	See Details
cross-compile (linux, ppc64le)	- 🔗	N/A	See Details
cross-compile (windows, 386)	- 🔗	N/A	See Details
cross-compile (windows, amd64)	- 🔗	N/A	See Details
build-package (deb)	- 🔗	N/A	See Details
build-package (rpm)	- 🔗	N/A	See Details
windows-msi	- 🔗	N/A	See Details
publish-check	- 🔗	N/A	See Details
publish-dev	- 🔗	N/A	See Details
publish-stable	- 🔗	N/A	See Details

⭕ build-and-test-windows workflow has finished in 3 seconds (42 minutes 29 seconds less than `main` branch avg.) and finished at 17th Jan, 2023.

Job	Failed Steps	Tests
windows-unittest	- 🔗	N/A	See Details
windows-unittest-matrix	- 🔗	N/A	See Details

✅ changelog workflow has finished in 2 minutes 13 seconds and finished at 17th Jan, 2023.

Job	Failed Steps	Tests
changelog	- 🔗	N/A	See Details

🔎 See details on Foresight

^{*You can configure Foresight comments in your organization settings page.}

evan-bradley · 2023-01-13T16:37:12Z

.chloggen/ottl-fix-ismatch.yaml

+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  - Affected components


What's the usual approach with changes to libraries like this? Should each impacted component get its own changelog entry? I'm good with doing it like this, but could also see the other side of things.

I don't think we have an established pattern. I did this once before in the 0.65.0 release.

bogdandrutu · 2023-01-21T19:11:55Z

pkg/ottl/ottlfuncs/func_is_match.go

@@ -38,6 +38,9 @@ func IsMatch[K any](target ottl.Getter[K], pattern string) (ottl.ExprFunc[K], er
 		if err != nil {
 			return nil, err
 		}
+		if val == nil {
+			return false, nil
+		}

 		switch v := val.(type) {


Not related to this PR, but keep in mind that this path is a slippery path, and will for sure hurt us. The fact that you accept multiple types instead of asking users to call this as IsMatch(..., string(value)) and enforce always having string here. The reason is because I feel like this will get duplicate in multiple places and will diverge.

You can at least start by adding "String()" func and call it on the value to avoid duplicate code.

I don't think we want to force users to use a String() Converter function every time they use IsMatch so I like that the function handles conversion for you. Conversion is only happening for the target, not the pattern.

If your concern is only the potential duplication of the type switch statement for strings, I can pull that out into a helper function for future reuse.

Let's start with avoiding potential duplication, I bet we already duplicate this.

Handle nil val in IsMatch

119be88

TylerHelmuth requested review from a team, bogdandrutu and evan-bradley as code owners January 13, 2023 16:25

github-actions bot assigned Aneurysm9 Jan 13, 2023

github-actions bot added the pkg/ottl label Jan 13, 2023

github-actions bot requested a review from kentquirk January 13, 2023 16:25

add changelog entry

6570825

frzifus approved these changes Jan 13, 2023

View reviewed changes

evan-bradley approved these changes Jan 13, 2023

View reviewed changes

TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Jan 17, 2023

TylerHelmuth requested a review from codeboten January 19, 2023 23:12

bogdandrutu reviewed Jan 21, 2023

View reviewed changes

bogdandrutu merged commit e797d0a into open-telemetry:main Jan 21, 2023

TylerHelmuth deleted the ottl-fix-ismatch branch January 21, 2023 19:36

plantfansam mentioned this pull request Jul 21, 2023

disregard #24427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pkg/ottl] Handle nil val in IsMatch #17572

[pkg/ottl] Handle nil val in IsMatch #17572

TylerHelmuth commented Jan 13, 2023

runforesight bot commented Jan 13, 2023 •

edited

`build-and-test-windows` duration(3 seconds) has decreased 42 minutes 29 seconds compared to main branch avg(42 minutes 32 seconds).

✅ check-links workflow has finished in 46 seconds (1 minute 14 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ tracegen workflow has finished in 56 seconds (1 minute 36 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ telemetrygen workflow has finished in 58 seconds (1 minute 39 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ prometheus-compliance-tests workflow has finished in 3 minutes 43 seconds (4 minutes 12 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ load-tests workflow has finished in 8 minutes 56 seconds (6 minutes 6 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ build-and-test workflow has finished in 40 minutes 11 seconds (8 minutes 44 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

⭕ build-and-test-windows workflow has finished in 3 seconds (42 minutes 29 seconds less than `main` branch avg.) and finished at 17th Jan, 2023.

✅ changelog workflow has finished in 2 minutes 13 seconds and finished at 17th Jan, 2023.

evan-bradley Jan 13, 2023

TylerHelmuth Jan 13, 2023

bogdandrutu Jan 21, 2023

TylerHelmuth Jan 21, 2023 •

edited

bogdandrutu Jan 21, 2023

[pkg/ottl] Handle nil val in IsMatch #17572

[pkg/ottl] Handle nil val in IsMatch #17572

Conversation

TylerHelmuth commented Jan 13, 2023

runforesight bot commented Jan 13, 2023 • edited

Foresight Summary

build-and-test-windows duration(3 seconds) has decreased 42 minutes 29 seconds compared to main branch avg(42 minutes 32 seconds).

✅ check-links workflow has finished in 46 seconds (1 minute 14 seconds less than main branch avg.) and finished at 13th Jan, 2023.

✅ tracegen workflow has finished in 56 seconds (1 minute 36 seconds less than main branch avg.) and finished at 13th Jan, 2023.

✅ telemetrygen workflow has finished in 58 seconds (1 minute 39 seconds less than main branch avg.) and finished at 13th Jan, 2023.

✅ prometheus-compliance-tests workflow has finished in 3 minutes 43 seconds (4 minutes 12 seconds less than main branch avg.) and finished at 13th Jan, 2023.

✅ load-tests workflow has finished in 8 minutes 56 seconds (6 minutes 6 seconds less than main branch avg.) and finished at 13th Jan, 2023.

✅ build-and-test workflow has finished in 40 minutes 11 seconds (8 minutes 44 seconds less than main branch avg.) and finished at 13th Jan, 2023.

⭕ build-and-test-windows workflow has finished in 3 seconds (42 minutes 29 seconds less than main branch avg.) and finished at 17th Jan, 2023.

✅ changelog workflow has finished in 2 minutes 13 seconds and finished at 17th Jan, 2023.

evan-bradley Jan 13, 2023

Choose a reason for hiding this comment

TylerHelmuth Jan 13, 2023

Choose a reason for hiding this comment

bogdandrutu Jan 21, 2023

Choose a reason for hiding this comment

TylerHelmuth Jan 21, 2023 • edited

Choose a reason for hiding this comment

bogdandrutu Jan 21, 2023

Choose a reason for hiding this comment

runforesight bot commented Jan 13, 2023 •

edited

`build-and-test-windows` duration(3 seconds) has decreased 42 minutes 29 seconds compared to main branch avg(42 minutes 32 seconds).

✅ check-links workflow has finished in 46 seconds (1 minute 14 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ tracegen workflow has finished in 56 seconds (1 minute 36 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ telemetrygen workflow has finished in 58 seconds (1 minute 39 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ prometheus-compliance-tests workflow has finished in 3 minutes 43 seconds (4 minutes 12 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ load-tests workflow has finished in 8 minutes 56 seconds (6 minutes 6 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

✅ build-and-test workflow has finished in 40 minutes 11 seconds (8 minutes 44 seconds less than `main` branch avg.) and finished at 13th Jan, 2023.

⭕ build-and-test-windows workflow has finished in 3 seconds (42 minutes 29 seconds less than `main` branch avg.) and finished at 17th Jan, 2023.

TylerHelmuth Jan 21, 2023 •

edited