-
Couldn't load subscription status.
- Fork 3.1k
[processor/transform] Create With*Functions factory options to provide custom OTTL funcs programatically.
#39966
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] Create With*Functions factory options to provide custom OTTL funcs programatically.
#39966
Conversation
WithAdditional*Funcs factory options and Validate*Statements to provide additional OTTL funcs programatically.WithAdditional*Funcs factory options and ValidateWithAdditionalFunctions to provide additional OTTL funcs programatically.
WithAdditional*Funcs factory options and ValidateWithAdditionalFunctions to provide additional OTTL funcs programatically.WithAdditional*Funcs factory options to provide additional OTTL funcs programatically.
|
Following up on the review request CC @edmocosta @TylerHelmuth @evan-bradley. This PR implements the approach discussed in #39641 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on, and sorry for the slow review.
Could we adjust the approach slightly?
- Make
LogFunctions,SpanFunctions, etc. exported at the top level of the transform processor module. - Instead of supplying additional functions, can we make it so the user specifies all functions they would like to include?
So the invocation would look like this:
NewFactory(
WithSpanFunctions(
transformprocessor.DefaultSpanFunctions(),
[]ottl.Factory[ottlspan.TransformContext]{FunctionOne, FunctionTwo, ...},
[]ottl.Factory[ottlspan.TransformContext]{FunctionThree, FunctionFour, ...},
),
)I think this has two advantages:
- Treats all the functions the same. The OTTL stdlib functions aren't anything special, so we shouldn't segment them in the code.
- Allows the component author to make it so only the functions they want are enabled in the transform processor. This could be useful if, for example, a distro author wants to remove
MD5for security reasons or standardize all statements to use the pattern replacement functions instead of the match functions.
I agree with @evan-bradley approach, and in addition to that I'd:
|
I thought about doing this, but I considered that we are likely to have other ways for users to add OTTL functions in the future (e.g. an "OTTL function" extension type, or functions loaded from config with wasm), and if we have this option pass a complete function list, it feels like that would have unclear semantics if combined with user-defined functions.
Maybe a compromise to avoid duplicate code would be to put the options in an internal package and reexpose them in
How would you combine this with @evan-bradley's request to pass in the complete function list? At that point every function would be "custom". |
Yes, I agree we should aim for consistent APIs, but I still don't think it should be enforced/defined by the OTTL package. It already provides the parsers/utilities that components need to use the language, but how they do that, is up to them. There are many different use-cases for OTTL at the moment (processor, connectors...), and it would be tricky to come up with some common options that perfectly fit all components. For example, providing the
That's a good question, I wish we could find a good solution for that. If I had to choose between both, I would rather start limiting it to additional functions than not having those logs. Another option would be keeping functions segmented ( |
6271568 to
5338205
Compare
@quentinmit That's a good consideration. Personally, I don't want to plan too far in advance for that since those ideas may not end up happening or may end up taking a different form than we'd expect. Additionally, I would personally be okay with
@edmocosta I think we could likely define these in a common way eventually, but for now I agree it's okay for these options to live in the transform processor. We can separate them into a different package down the road once we have a better understanding of how they're used.
I think we could simply emit a log if any of the Overall I don't think segmenting the core/user-defined functions in the |
Agreed, those extra logs that I mentioned would be useful for troubleshooting, so I think they should be debug logs. Custom collector distributions using this new API probably wouldn't want them to be printed in any other level as well. |
WithAdditional*Funcs factory options to provide additional OTTL funcs programatically.With*Funcs factory options to provide custom OTTL funcs programatically.
With*Funcs factory options to provide custom OTTL funcs programatically.With*Functions factory options to provide custom OTTL funcs programatically.
|
I implemented all the last details and comments on this PR. This PR is ready for another review. The A summary of the updates :
|
22c2d08 to
7779541
Compare
| // WithDataPointFunctions set ottl datapoint functions in resulting processor. | ||
| func WithDataPointFunctions(dataPointFunctions ...[]ottl.Factory[ottldatapoint.TransformContext]) FactoryOption { | ||
| return func(factory *transformProcessorFactory) { | ||
| factory.dataPointFunctions = createFunctionsMap(dataPointFunctions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have it so the functions are added to the existing map instead of overriding it? I think the UX will be better if the user can call the With*Functions options multiple times so they can easily add functions from different sources without manually merging them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have it so the functions are added to the existing map instead of overriding it?
Yes, that can work too. I updated the FactoryOptions in my last commit cc848b6 .
The new behaviour overrides the "default" functions on its first use of any With*Functions and every subsequent use merges the new functions to the "current" functions map.
Before Last Update
For comparison, this is the previous way of setting functions.
NewFactoryWithOptions(
WithLogFunctions(
transformprocessor.DefaultLogFunctions(),
[]ottl.Factory[ottllog.TransformContext]{FunctionOne, FunctionTwo, ...},
[]ottl.Factory[ottllog.TransformContext]{FunctionThree, FunctionFour, ...},
),
)
Now functions can be set like this :
After Last Update
NewFactoryWithOptions(
WithLogFunctions(transformprocessor.DefaultLogFunctions()),
WithLogFunctions([]ottl.Factory[ottllog.TransformContext]{FunctionOne, FunctionTwo, ...}),
WithLogFunctions([]ottl.Factory[ottllog.TransformContext]{FunctionThree, FunctionFour, ...}),
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I missed the variadic argument in the With*Functions functions. I think both of these are roughly equivalent, though I still have a slight preference for the second pattern outlined in your comment.
@edmocosta @TylerHelmuth Please let us know if you feel differently about how this should look.
151f746 to
cc848b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your persistence through this review process @franciscovalentecastro. This looks good to me, just one request for a few additional test cases.
|
@edmocosta @TylerHelmuth Please take a look when you are able. @quentinmit your review would be welcome here as well. |
… from `create*Processor` when setting custom functions.
… of `WithAdditional*Functions`. Create debug log.
… from `make checkapi`.
…ssert unexported properties of config.
… multiple times.
Co-authored-by: Edmo Vamerlatti Costa <[email protected]>
…ctions log messages and fix typo.
…tered non-default functions.
6d4f449 to
825d3e4
Compare
|
@edmocosta could you give this another look when you are able? |
|
@evan-bradley @edmocosta @quentinmit Thank you 🙏 for all the reviews, comments and design guidance throughout this FR / PR ! |
…ide custom OTTL funcs programatically. (open-telemetry#39966) Co-authored-by: Edmo Vamerlatti Costa <[email protected]> Co-authored-by: Evan Bradley <[email protected]>
Description
Create
With*Functionsfactory options to provide custom OTTL functions for logs, metrics or traces to the resulting processor. Also created the "exported"Default*Functionsfunctions that can be used to provide a "subset" of the default functions used in the the transform processor.Since in the current collector design the
Configdoesn't have access to theFactory, theConfig.Validatemethod can't access the additional OTTL functions set in theNewFactory. I have found two ways to circumvent this :*Functionsto theConfigstruct to use in theValidate()method.ValidateFunctionsmethods that can be used to create a customConfig.Validate().Some details about the solution :
transformprocessor the user needs to create aNewFactoryproviding the custom OTTL functions.Additional*Funcsconfig properties to provide additional OTTL funcs programatically. #39641 (comment)Usage
The factory options can be used like the following example :
Link to tracking issue
Note : There is a similar followup proposed update for the
processor/filter.Testing
Documentation
With*Functions,Default*FunctionsandFactoryWithOptionsfunctions.