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/ottl] Fix the factory name for the limit function #21921

Merged
merged 1 commit into from
May 15, 2023

Conversation

swiatekm
Copy link
Contributor

Description:
The factory name for the limit function is uppercase, but OTTL requires functions to be lowercase. As a result, it can't be used in OTTL statements.

Link to tracking Issue: #21920

Testing:
I built the contrib binary and tested against the config from the issue.
I wanted to add a unit test for this as well, but wasn't sure where it should live. Seems like we should have tests like this (which verify that a particular invocation of a function or converter can indeed be parsed end-to-end) for every function.

@swiatekm swiatekm requested a review from a team May 15, 2023 10:04
@swiatekm swiatekm changed the title Fix the factory name for the limit function [pkg/ottl] Fix the factory name for the limit function May 15, 2023
@github-actions github-actions bot requested a review from kentquirk May 15, 2023 10:04
@evan-bradley
Copy link
Contributor

Thank you for catching this!

I agree that we should have a test covering this fix. Typically we ask anyone who adds a function to the transform processor to add a statement using the function to the processing tests for each context (e.g. Test_ProcessTraces_TraceContext in processor/transformprocessor/internal/traces/processor_test.go), though now that I look at the coverage for existing functions, it looks like we're missing quite a few, including limit. For this PR, I think it would be sufficient to add a transform processor test covering limit in these tests, since we want that anyway and it would cover the fix without being a significant ask from you.

Going forward we ought to have tests within the OTTL package that cover parsing each function to test the name and arguments. I think writing a test parser for a specific context (e.g. spans) that tests parsing the function in isolation would be the cleanest approach, but having package-wide tests instead or in addition could also be an option. @TylerHelmuth let me know if you think we should take another approach, otherwise I'll open a PR to help improve our coverage here.

@github-actions github-actions bot added the processor/transform Transform processor label May 15, 2023
@swiatekm
Copy link
Contributor Author

Allright, added a simple test for limit for each signal type in the transform processor tests.

@TylerHelmuth
Copy link
Member

@evan-bradley agreed, OTTL should be able to test for this kind of thing itself. Using the trasnformprocessor tests as "e2e" tests worked well while OTTL was transitioning into its own pkg and while the transformprocessor was determining its function names. Now that the factory defines the function names it would be good to have some "e2e" tests in the pkg as well.

We shouldn't get rid of the transfomprocessor tests though, they still help the transformprocessor know it is going to work.

@TylerHelmuth TylerHelmuth linked an issue May 15, 2023 that may be closed by this pull request
@TylerHelmuth TylerHelmuth merged commit 182d8c2 into open-telemetry:main May 15, 2023
@github-actions github-actions bot added this to the next release milestone May 15, 2023
@swiatekm swiatekm deleted the fix/ottl/limit branch May 15, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] limit function can't be used
4 participants