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.

Sign up for GitHub

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

Jump to bottom

[pkg/ottl] Create "e2e" tests in the package #28642

Closed
Tracked by #28892
TylerHelmuth opened this issue Oct 26, 2023 · 6 comments · Fixed by #30152
Closed
Tracked by #28892

[pkg/ottl] Create "e2e" tests in the package #28642

TylerHelmuth opened this issue Oct 26, 2023 · 6 comments · Fixed by #30152
Labels
Contribfest enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed pkg/ottl priority:p2 Medium

Comments

@TylerHelmuth
Copy link
Member

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

Currently OTTL depends on the transformprocessor's processor_test.go tests in order to actually test parsing and execution of complete statements. The OTTL pkg only tests parsing and individual functions, but it does not test everything all together.

Describe the solution you'd like

A new set of tests that create a payload and tests execution of statements.

Describe alternatives you've considered

No response

Additional context

We should not remove the transformprocessor tests.
@bryan-aguilar
Copy link
Contributor

Do you think this is a good fit for adding to the testbed?

@jiekun
Copy link
Member

jiekun commented Oct 30, 2023

+1 for adding to testbed. I tried it with tailsampling processor, and I think it's simple (enough) to add new components.

(BTW I feel like it's a good-first-issue if someone wants to contribute?)

@github-actions github-actions bot mentioned this issue Oct 31, 2023
Closed
@TylerHelmuth
Copy link
Member Author

I'm gonna level with ya, I don't know anything about testbed. Can you explain how it helps solve our problem?

I was imagining a similar test structure that we use in the transformprocessor where we create the data using pdata and then pass it in to OTTL. In this instance instead of calling .Consume, which is a processor thing, we would create a new parser, call ParseStatements, call NewStatements, and then call Execute. For the purposes of OTTL this tests our primary external functions and the ability to manipulate data without requiring a mock collector/loadgen behind the scenes.

@TylerHelmuth TylerHelmuth mentioned this issue Nov 5, 2023
Open
11 tasks
@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed good first issue Good for newcomers labels Dec 13, 2023
@TylerHelmuth TylerHelmuth mentioned this issue Dec 18, 2023
Merged
TylerHelmuth added a commit that referenced this issue Dec 18, 2023
@TylerHelmuth
[pkg/ottl] Don't check that all keys are used (#30047)
f9807d0 
**Description:** 
The desire to validate both path segments AND keys led to a bug where a
totally valid statement like

`replace_match(body["metadata"]["uid"], "*", "12345")` 

fails since only `metadata` is checked during parsing; `uid` is checked
during hot-path get/set of the value.

Failing such a statement is not the intention of
#22744
and it was incorrect to fail such a statement. In fact, I believe
validating the key's use in the context will be even more complex once
we introduce dynamic indexing.

For these reasons, this PR removes Key validation for now. If, in the
future, we want to re-add these validations, our Interfaces allow that.

**Link to tracking Issue:** 

#22744

#30051

**Testing:** <Describe what testing was performed and which tests were
added.>
Unit tests

Also we wouldve caught this issue earlier if we had an e2e test that did
complex indexing but unfortunately we did in the transform processor.
All the more reason to implement
#28642.
@TylerHelmuth
Copy link
Member Author

@evan-bradley how extensive do you think we should make these tests? Should we test every function via every context or is sticking with a single context enough? Should we test every path of every context, or is the context's existing newPathGetSetter tests enough?

@TylerHelmuth
Copy link
Member Author

I do not believe we need to e2e tests the different OTTL Contexts we provide. Each Context already has extensive testing that the Getters and Setters returned by the context manipulate the underlying telemetry correctly.

Similarly, I do not believe we need to e2e test parsing statements. The tests in parser_test.go extensively test each part of our grammar.

What we do need to test with e2e tests is that:

  1. The OTTL features behave/interact correctly. This includes things like Where Clauses gating function execution, complex indexing, function composition, etc.
  2. Each one of our ottlfuncs functions can be parsed and invoked (the ottlfuncs unit tests do not check this).

This was referenced Dec 20, 2023
Merged
Merged
@evan-bradley
Copy link
Contributor

All of that sounds good to me.

TylerHelmuth added a commit that referenced this issue Jan 8, 2024
@TylerHelmuth
[pkg/ottl] Add e2e tests (#30152)
e5dfb80 
**Description:** 
Adds e2e tests for OTTL functionality and all `ottlfuncs` functions.

Includes the bug fix from
#30151

**Link to tracking Issue:** <Issue number if applicable>
Closes
#28642

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
Updated contributing doc
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
@TylerHelmuth @cparkins
[pkg/ottl] Don't check that all keys are used (open-telemetry#30047)
fbdc8ee 
**Description:** 
The desire to validate both path segments AND keys led to a bug where a
totally valid statement like

`replace_match(body["metadata"]["uid"], "*", "12345")` 

fails since only `metadata` is checked during parsing; `uid` is checked
during hot-path get/set of the value.

Failing such a statement is not the intention of
open-telemetry#22744
and it was incorrect to fail such a statement. In fact, I believe
validating the key's use in the context will be even more complex once
we introduce dynamic indexing.

For these reasons, this PR removes Key validation for now. If, in the
future, we want to re-add these validations, our Interfaces allow that.

**Link to tracking Issue:** 

open-telemetry#22744

open-telemetry#30051

**Testing:** <Describe what testing was performed and which tests were
added.>
Unit tests

Also we wouldve caught this issue earlier if we had an e2e test that did
complex indexing but unfortunately we did in the transform processor.
All the more reason to implement
open-telemetry#28642.
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Jan 10, 2024
@TylerHelmuth @cparkins
[pkg/ottl] Add e2e tests (open-telemetry#30152)
d8803e8 
**Description:** 
Adds e2e tests for OTTL functionality and all `ottlfuncs` functions.

Includes the bug fix from
open-telemetry#30151

**Link to tracking Issue:** <Issue number if applicable>
Closes
open-telemetry#28642

**Testing:** <Describe what testing was performed and which tests were
added.>

**Documentation:** <Describe the documentation added.>
Updated contributing doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Contribfest enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed pkg/ottl priority:p2 Medium
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[pkg/ottl] Add e2e tests TylerHelmuth/opentelemetry-collector-contrib
4 participants
@evan-bradley @TylerHelmuth @jiekun @bryan-aguilar