Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/stanza] Upgrade to github.com/antonmedv/expr v1.12.7 breaks tests #24575

Closed
songy23 opened this issue Jul 26, 2023 · 3 comments · Fixed by #24648
Closed

[pkg/stanza] Upgrade to github.com/antonmedv/expr v1.12.7 breaks tests #24575

songy23 opened this issue Jul 26, 2023 · 3 comments · Fixed by #24648
Assignees
Labels
bug Something isn't working pkg/stanza

Comments

@songy23
Copy link
Member

songy23 commented Jul 26, 2023

Component(s)

pkg/stanza

What happened?

See CI failure: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/5650490793/job/15307018157?pr=24536

To reproduce, update github.com/antonmedv/expr to v1.12.7 in pkg/stanza/go.mod, then run the tests with make test.

Collector version

n/a

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

@songy23 songy23 added bug Something isn't working needs triage New item requiring triage labels Jul 26, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski
Copy link
Member

It looks like the expr module added a new key word, "env" to its grammar.

Unfortunately, we already had added some custom behavior for the same keyword.

The test failures quite clearly appear to be related:

--- FAIL: TestAttributer (0.00s)
    --- FAIL: TestAttributer/AddAttributeEnv (0.00s)
        attributer_test.go:85: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/operator/helper/attributer_test.go:85
            	Error:      	Received unexpected error:
            	            	render embedded expression: reflect: call of reflect.Value.Call on map Value (1:1)
            	            	 | env("TEST_METADATA_OPERATOR_ENV")
            	            	 | ^
            	Test:       	TestAttributer/AddAttributeEnv
--- FAIL: TestExprString (0.00s)
    --- FAIL: TestExprString/12 (0.00s)
        expr_string_test.go:100: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/operator/helper/expr_string_test.go:100
            	Error:      	Received unexpected error:
            	            	render embedded expression: reflect: call of reflect.Value.Call on map Value (1:1)
            	            	 | env('TEST_EXPR_STRING_ENV')
            	            	 | ^
            	Test:       	TestExprString/12
--- FAIL: TestIdentifier (0.00s)
    --- FAIL: TestIdentifier/AddAttributeEnv (0.00s)
        identifier_test.go:85: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/operator/helper/identifier_test.go:85
            	Error:      	Received unexpected error:
            	            	render embedded expression: reflect: call of reflect.Value.Call on map Value (1:1)
            	            	 | env("TEST_METADATA_OPERATOR_ENV")
            	            	 | ^
            	Test:       	TestIdentifier/AddAttributeEnv
--- FAIL: TestTransformer (0.00s)
    --- FAIL: TestTransformer/NoMatchEnv (0.00s)
        logger.go:130: 2023-07-24T22:42:41.909Z	ERROR	Running expressing returned an error%!(EXTRA zapcore.Field={error 26 0  reflect: call of reflect.Value.Call on map Value (1:1)
             | env("TEST_FILTER_OPERATOR_ENV") == "bar"
             | ^})	{"operator_id": "test", "operator_type": "filter"}
        filter_test.go:178: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/operator/transformer/filter/filter_test.go:178
            	Error:      	Not equal: 
            	            	expected: false
            	            	actual  : true
            	Test:       	TestTransformer/NoMatchEnv
--- FAIL: TestTransformer (0.01s)
    --- FAIL: TestTransformer/MatchEnv (0.00s)
        router_test.go:215: 
            	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/pkg/stanza/operator/transformer/router/router_test.go:215
            	Error:      	Not equal: 
            	            	expected: map[string]int{"output1":1}
            	            	actual  : map[string]int{"output2":1}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1,3 +1,3 @@
            	            	 (map[string]int) (len=1) {
            	            	- (string) (len=7) "output1": (int) 1
            	            	+ (string) (len=7) "output2": (int) 1
            	            	 }
            	Test:       	TestTransformer/MatchEnv

I'm looking into it, but we likely need to lock in the version of the module for now.

@songy23 songy23 removed the needs triage New item requiring triage label Jul 26, 2023
djaglowski added a commit that referenced this issue Jul 31, 2023
Fixes
#24575

This upgrade involves a workaround for a grammar conflict introduced in
[this PR](https://github.com/antonmedv/expr/pull/382/files).

`pkg/stanza` previously added a custom function to the syntax called
`env()`, which allows users to fetch values from OS environment
variables.

The latest version of the `expr` module introduced its own keyword
`env`, but the meaning and usage were different. Still, the overlap was
enough to cause failures in the compilation and rendering of
expressions.

The new syntax was introduced as a [Membership
Operator](https://expr.medv.io/docs/Language-Definition#membership-operator),
which means that the proper way to use it is with square brackets,
`env[]`. This resulted in our custom `env()` syntax being flagged as
invalid.

More importantly, the meaning of the new `env[]` is not the same. While
we use `env()` to lookup values from the OS environment, `env[]` is
meant to pull values from an in-memory map. This map allows for
customizations of the syntax, such as those that allow users to
reference "body" or "attributes" within an expression, but it does not
contain OS environment variables. Therefore, accessing it with the new
syntax is of limited value to users.

The solution used here is a
[Patch](https://github.com/antonmedv/expr/blob/d2100ece96affe6766d97e2cf0e4ca4d145f0d30/expr.go#L98)
option which allows us to inspect and modify elements of the grammar as
an expression is being compiled. Fortunately, `env()` is understood to
be a function call, while `env[]` is otherwise. This makes it possible
to detect usage of our custom syntax and rename the function internally
without any impact to the user.

The PR also standardizes all uses of `expr.Compile` within `pkg/stanza`
to ensure they are using the new patch option.
@antonmedv
Copy link

Hmm. env is very popular keyword. I will change it to something less popular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg/stanza
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants