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] Remove functions option from config #12973

Conversation

TylerHelmuth
Copy link
Member

@TylerHelmuth TylerHelmuth commented Aug 4, 2022

Description:
The transformprocessor had a setting in configuration called functions that was "no-op". Setting this field would actually break the transform processor because the TQL would have no working functions. The long term intention was that users would be able to register their own functions to be usable with the TQL, but there is no implementation to do that currently.

This PR removes the unused config option, which will work towards providing a declarative config syntax.

Link to tracking Issue:
Related to #11852

Testing:
Ran unit tests

Documentation:
This field is not currently documented. Since the processor is alpha, we can make breaking config changes without considering backwards compatibility.

@TylerHelmuth TylerHelmuth added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 4, 2022
@TylerHelmuth TylerHelmuth requested a review from a team August 4, 2022 21:25
@TylerHelmuth
Copy link
Member Author

/cc @kentquirk

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Is this a breaking change?

@TylerHelmuth
Copy link
Member Author

Is this a breaking change?

Technically yes. But since this component is alpha we can still make config changes in breaking ways. Also, the config option, when used, breaks the processor.

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Aug 5, 2022

@mx-psi Actually the mapstructure:"-" means it is ignored right? If that's the case the setting isn't settable via config and is only used to access functions, which is unnecessary.

@mx-psi
Copy link
Member

mx-psi commented Aug 5, 2022

@mx-psi Actually the mapstructure:"-" means it is ignored right? If that's the case the setting isn't settable via config and is only used to access functions, which is unnecessary.

Right, and it's a private field, so probably even if it had a tag you wouldn't be able to set it via mapstructure, and it does not affect the Go API 👍 You have convinced me :)

Copy link
Member

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I'm late to the party, but this is good. One question, but I'm happy if the answer is "it's fine."

@@ -23,13 +23,13 @@ import (
)

func Test_DefaultFunctions(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.

This tests if there are any unexpected values, but not if any are missing.

Is there any percentage in also turning the test around?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kentquirk I think technically the test would pass incorrectly if actual contained the same function definition an exact amount of times to match the length of expected and the function name was in expected. I could add more logic into the check, but it might be overcomplicating things. The main goal of the test is to ensure the signal-specific Functions values uses common.Functions.

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Aug 15, 2022
@djaglowski djaglowski merged commit a5b841e into open-telemetry:main Aug 15, 2022
@TylerHelmuth TylerHelmuth deleted the issue-11852-remove-func-from-config branch August 15, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants