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] Add ability to concat strings #12476

Closed
TylerHelmuth opened this issue Jul 14, 2022 · 10 comments · Fixed by #13120 or #13553
Closed

[processor/transform] Add ability to concat strings #12476

TylerHelmuth opened this issue Jul 14, 2022 · 10 comments · Fixed by #13120 or #13553
Assignees
Labels
good first issue Good for newcomers priority:p2 Medium processor/transform Transform processor

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jul 14, 2022

Is your feature request related to a problem? Please describe.
There are situations where a user might need to combine existing fields to be used as an argument in an Invocation. Here is an example.. At the moment the TQL has no build-in capability to do this and the transform processor has not function that can do it.

Describe the solution you'd like
I think we have 2 options.

  1. Add a "Concat" factory to the transform processor that takes 2 string arguments and returns a concatenation of the 2 string. This is a solution that should be implemented now.
  2. Add something to the TQL so it knows how to do this natively. We should do this, but in the future. For now, to enhance the transform processor, we'll use a Factory.

I'm leaning towards option 1, as it is simpler to implement and factories are already a defined pattern.

@TylerHelmuth TylerHelmuth added priority:p2 Medium processor/transform Transform processor labels Jul 14, 2022
@TylerHelmuth
Copy link
Member Author

@kentquirk what do you think? If we go anywhere near option 2 it needs to wait on #11751 lol

@kentquirk
Copy link
Member

In the long run, I'd prefer to actually give TQL a full expression parser that can do math and concatenations and basic language operations -- and probably also a few embedded functions in TQL for things like string processing and type conversion.

But I don't think that a Concat factory in the transform processor gets in the way of that, so if it solves an immediate problem I would support the idea.

@TylerHelmuth
Copy link
Member Author

Agreed.

@TylerHelmuth TylerHelmuth added help wanted Extra attention is needed good first issue Good for newcomers labels Jul 14, 2022
@TylerHelmuth
Copy link
Member Author

Updated Issue description.

@evan-bradley
Copy link
Contributor

I can take this one.

I think it may make sense to expand the scope a little bit. Would it make sense to:

  1. Make Concat variadic or accept a list of strings, which would allow concatenating multiple strings without needing to chain Concat calls.
  2. Use Go templates to process a template string given data available for the signal being processed. This would be optimal for use and readability unless there is an issue using Go templates in this context.

I realize that this issue is supposed to be a stopgap until the TQL supports more string operations, so if a more powerful solution would be better suited for that effort, then I think simply concatenating two strings works. Additionally, I'm not sure if the parser can currently handle either of those cases, so keeping the factory simple may be necessary.

@TylerHelmuth
Copy link
Member Author

Make Concat variadic or accept a list of strings, which would allow concatenating multiple strings without needing to chain Concat calls.

Great idea. The grammar knows how to accept string arrays so this is just as straight forward as 2 strings.

Use Go templates to process a template string given data available for the signal being processed. This would be optimal for use and readability unless there is an issue using Go templates in this context.

I'm not following what you mean here.

@evan-bradley
Copy link
Contributor

I'm not following what you mean here.

Using Go templates could look like the following:

set(name, Template("{{.name}}-{{.attributes["book_genre"]}}")) # Sets name to "GetReview-Fiction"

The data provided to fill the template would just come from the signal fields.

Looking at this now, I think just providing a list to the Concat function will work fine, since it will be more straightforward to implement and arguably reads better.

@TylerHelmuth
Copy link
Member Author

I agree. Especially because templating looks the same as building concat directly into the language: set(name, name + attributes["book_genre"]). But I think language-aware concat can come later.

@evan-bradley
Copy link
Contributor

Agreed. Please assign this to me, I should be able to implement the factory relatively quickly.

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Jul 25, 2022

@evan-bradley, the factory function will probably need to accept Getters instead of strings, so that functions and paths can be used. I don't think the grammar supports arrays of getters yet, but it could be made to do so. That would be an update to pkg/telemetryquerylanguage, and should be a separate PR.

evan-bradley added a commit to evan-bradley/opentelemetry-collector-contrib that referenced this issue Jul 26, 2022
evan-bradley added a commit to evan-bradley/opentelemetry-collector-contrib that referenced this issue Aug 1, 2022
codeboten pushed a commit that referenced this issue Aug 8, 2022
… functions (#12706)

This allows TQL functions to receive an array of Getters as a parameter.

This is a prerequisite for #12476, which will need to concatenate strings and path references to strings and therefore requires the ability to receive an array containing more than one primitive type.

Co-authored-by: Evan Bradley <[email protected]>
Co-authored-by: Tyler Helmuth <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers priority:p2 Medium processor/transform Transform processor
Projects
None yet
3 participants