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

[processor/transform] Flatten functions #10489

Conversation

TylerHelmuth
Copy link
Member

Description:
This PR is the next step towards our ongoing function refactor. This PR moves each function into its own file with its own test file. This makes it easier to:

  • Find and understand a specific function.
  • Test a specific function. Notably we can now unit test functions without needing to call NewFunctionCall. For functions in common, they can now be tested independently of any specific signal.
  • Adding a new functions. All new functions should be added with exactly 1 function file, 1 test file, and 1 modification to the appropriate functions.go to register it. A CONTRIBUTING.md will be added in a separate PR.

This PR does introduce a significant number of duplicate tests. Once this PR is merged I plan to remove all the duplicate tests from the signal-specific functions.go files.

Link to tracking Issue:
#10101

Testing:
Updated unit tests

Documentation:
This is a no-functionality change, so all existing documentation is correct.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 1, 2022
@TylerHelmuth TylerHelmuth requested a review from a team June 1, 2022 17:57
@TylerHelmuth
Copy link
Member Author

/cc @anuraaga

@TylerHelmuth TylerHelmuth requested a review from djaglowski as a code owner June 1, 2022 22:27
"go.opentelemetry.io/collector/pdata/pmetric"
)

func Test_convert_gauge_to_sum(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there's really a benefit to matching the test name style (snake case). It should be easy enough to correlate using normal syntax.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention was actual to match the query language syntax, but I will use the actual Go function name instead bc that is what is being tested.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djaglowski updated test function names

@djaglowski djaglowski merged commit ea61fb3 into open-telemetry:main Jun 8, 2022
@TylerHelmuth TylerHelmuth deleted the issue-10101-refactor-functions-flat branch June 8, 2022 17:58
kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
* Add all existing scenarios to reflection

* Update tests to only test NewFunctionCall

* Break out switch statements

* Small adjustments

* Update changelog

* Added testhelper

* moved functions into their own files

* added tests for keep_keys and limit

* Switched tests to use generic ctx instead of trace

* updated more tests

* removed all instances of ptrace from common

* fix merge

* flattened metric functions

* Simplify test context

* Fix lint issues

* Fix lint issues

* Fix lint issues

* ran make gotidy

* Update test function names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants