processor/transform Add implementation of query processing #7129
Conversation
anuraaga
force-pushed
the
processor-expression-trace-impl
branch
from
f68bb36
to
83a762b
Compare
|
@bogdandrutu @punya @Aneurysm9 - friendly ping for your reviews! |
| p, err := factory.CreateTracesProcessor(context.Background(), component.ProcessorCreateSettings{}, p0, consumertest.NewNop()) | ||
| assert.NoError(t, err) | ||
|
|
||
| td := constructTraces() |
There was a problem hiding this comment.
Very hard to read the test since I have to jump in 5 places (random number) to understand what is the input and how the checks that you have later make sense.
I prefer tests to have clear "input" and "output" expectations in one place.
| // limitations under the License. | ||
|
|
||
| // struct tags are special format so disable govet | ||
| // nolint:govet |
There was a problem hiding this comment.
I would disable this at the struct level, and let it run for the rest of the code.
| }, | ||
| }) | ||
| assert.NoError(t, configtest.CheckConfigStruct(cfg)) | ||
| } | ||
|
|
||
| func TestFactory_LoadConfig(t *testing.T) { |
There was a problem hiding this comment.
The way how this is written, is more or less a test for the processor now. May consider to add a Equal method on the Query to simplify load config test?
| "github.com/open-telemetry/opentelemetry-collector-contrib/processor/transformprocessor/internal/common" | ||
| ) | ||
|
|
||
| var registry = make(map[string]interface{}) |
There was a problem hiding this comment.
We try to avoid global variables, may not be a bad idea to pass "availableFuncs" instead.
| } | ||
| } | ||
|
|
||
| func constructTraces() pdata.Traces { |
There was a problem hiding this comment.
We have a "helper" package for this https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/coreinternal/testdata
|
|
||
| // Query holds a top level Query for processing trace data. A Query is a combination of a function | ||
| // invocation and the condition to match telemetry for invoking the function. | ||
| type Query struct { |
There was a problem hiding this comment.
Confused about multiple Query structs.
| function, err := newFunctionCall(parsed.Invocation) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| condition, err := newConditionEvaluator(parsed.Condition) |
There was a problem hiding this comment.
Not sure this is ok, if the "funcs" have state for example you force to have them shared across multiple instances of the processor. So I think you should parse the query here in common.Query(). then have a "Validate" to validate that funcs are registered/available (you can do that here as well). But the construction of the funcs probably need to happen in the constructor of the processor.
|
@alolita very hard to find 1-2 hours block to do a 2500 lines code review, that is why I am keep saying that we need to split PRs. Here we could have easily have a simple PR that adds the query parser. then the registry, then plug everything. |
Thanks @bogdandrutu - it's helpful to get a suggestion on how to split up the PR to make sure not to split it in unhelpful ways. I will split along these lines |
anuraaga
force-pushed
the
processor-expression-trace-impl
branch
from
83a762b
to
9a4e8be
Compare
anuraaga
changed the title
|
@bogdandrutu I split this down to only the parser logic |
bogdandrutu
added
the
Skip Changelog
Description:
Adds the business logic for parsing queries
Link to tracking Issue: open-telemetry/opentelemetry-collector#4444
Testing: Unit tests
Documentation: README