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] Add rename function #31901

Closed
aleksmaus opened this issue Mar 21, 2024 · 11 comments
Closed

[pkg/ottl] Add rename function #31901

aleksmaus opened this issue Mar 21, 2024 · 11 comments
Assignees
Labels
discussion needed Community discussion needed enhancement New feature or request pkg/ottl Stale

Comments

@aleksmaus
Copy link
Contributor

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

It might be useful to be able to rename the field with one call which currently requires two calls: set and delete_key.

This follows the initial proposal for strategy resolution for the set function by @michalpristas here #31808

Related to the initial implementation here: #31769

Describe the solution you'd like

For example
Instead of these two of calls

  - set(attributes["destination"], attributes["source"])
  - delete_key(attributes, "source")

Use

rename(attributes["destination"], attributes, "source")

Documentation

rename

rename(target, source_map, source_key, [Optional] ignore_missing = true, [Optional] conflict_strategy = upsert)

The rename function combines set and delete_key calls in one function. It creates the target field with the source_key value from source_map and it deletes the source_key from source_map.

target is a path expression to a telemetry field. source_map is a path expression to a pcommon.Map type field. source_key is a string key for source_map.

How the rename function behaves is controlled by the optional ignore_missing and conflict_strategy arguments.

ignore_missing is optional boolean argument that specifies what happens when the source_key is missing from the source_map. It is set to true by default.

  • The true value results in no changes if the source_key key doesn't exists in the source_map
  • The false value results in error if the source_key key doesn't exists in the source_map

conflict_strategy is an optional string parameter that specifies the conflict resolution strategy for the target.
Valid values are upsert, fail, and insert. By default, it is set to upsert.

  • The upsert overwrites the target value if it is already present.
  • The fail returns an error if target is already present.
  • The insert results in no changes if target is already present.

Examples:

  • rename(attributes["destination"], body, "source")
  • rename(attributes["destination"], attributes, "source", false)
  • rename(attributes["destination"], attributes, "source", true, "insert")

Describe alternatives you've considered

No response

Additional context

The function signature could have been better if we could do something like this
rename(attributes["destination"], body["source"])

Still leaning this product, didn't find a way to figure out the source key from expression like this.

@aleksmaus aleksmaus added enhancement New feature or request needs triage New item requiring triage labels Mar 21, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@Frapschen Frapschen removed the needs triage New item requiring triage label Mar 22, 2024
@TylerHelmuth
Copy link
Member

TylerHelmuth commented Mar 22, 2024

On one hand we have an existing solution for renaming an attribute and it is the same solution that attributesprocessor and resourceprocessor use. On the other hand, it'd be nice to do it in 1 statement and that's definitely something an OTTL function could encapsulate.

If we were to introduce a rename function, I think we would need to keep it very targeted to renaming an attribute key and not introduce any concept of the value anywhere; as soon as setting a value gets involved I think the 2 statement method is clearest as it lets you use Where clauses to handle any nuance.

If only changing the key name, I think the function would look like rename(attributes, "OldName", "NewName"). That ends up being very close to replace_all_patterns(attributes, "key", "old-name", "NewName") so I'm not sure we need it except for discoverability.

@TylerHelmuth TylerHelmuth added the discussion needed Community discussion needed label Mar 22, 2024
@aleksmaus
Copy link
Contributor Author

If only changing the key name, I think the function would look like rename(attributes, "OldName", "NewName"). That ends up being very close to replace_all_patterns(attributes, "key", "old-name", "NewName") so I'm not sure we need it except for discoverability.

Thank you for the prompt reply!
Looks like replace_all_patterns already covers the basic rename case that works on the same map.

@AlexanderWert @michalpristas if you have any more thoughts on this let me know.
Will close the issue and the PR if I don't hear any objections within this week then.

@AlexanderWert
Copy link
Member

@TylerHelmuth Thanks for the clarification!

If only changing the key name, I think the function would look like rename(attributes, "OldName", "NewName"). That ends up being very close to replace_all_patterns(attributes, "key", "old-name", "NewName") so I'm not sure we need it except for discoverability.

@TylerHelmuth What is the default behavior with replace_all_patterns(attributes, "key", "old-name", "NewName") if an attribute with the name NewName already exists?

That ends up being very close to replace_all_patterns(attributes, "key", "old-name", "NewName") so I'm not sure we need it except for discoverability.

IMHO it would be beneficial for the usability to have a more intuitive name for renaming an attribute. As you wrote replace_all_patterns(attributes, "key", "old-name", "NewName") is not very discoverable / intuitive for that use case.

@TylerHelmuth
Copy link
Member

What is the default behavior with replace_all_patterns(attributes, "key", "old-name", "NewName") if an attribute with the name NewName already exists?

Good question. I believe it will cause NewName's original value to be overwritten by the value at old-name. I agree that this isn't well documented and wasn't the original intention behind adding key support to replace_all_patterns.

@aleksmaus
Copy link
Contributor Author

Good question. I believe it will cause NewName's original value to be overwritten by the value at old-name. I agree that this isn't well documented and wasn't the original intention behind adding key support to replace_all_patterns.

The initial PR had exactly the simplified version of rename just supporting the key renaming in the same map. I can rollback the latest changes and have just simple replace(map, source_key, dest_key)
It also adds two optional parameters

[Optional] ignore_missing = true, [Optional] conflict_strategy = upsert

The conflict_strategy follows this proposal for set #31808

@aleksmaus
Copy link
Contributor Author

Reverted the latest changes back to to the initial rename function implementation, updated README.md

rename

rename(map, field, targetField, Optional[ignore_missing], Optional[conflict_strategy] )

The rename function renames the map field to targetField.

map is a path expression to a pcommon.Map type field. field is a string value of the map key that is being renamed. targetField is a string value that the field is renamed to.

This combines what previously required two calls of set following with delete_key in one function call.

How the rename function behaves is controlled by the optional ignore_missing and conflict_strategy arguments.

ignore_missing is optional boolean argument, that specifies what happens when the field is missing from the map. It is set to true by default.

  • The true value results in no changes to the map if the field key doesn't exists in the map
  • The false value results in error if the field key doesn't exists in the map

conflict_strategy is an optional string paramater that specifies the conflict resolution strategy for the targetField.
Valid values are upsert, fail, and insert. By default, it is set to upsert.

  • The upsert overwrites the targetField if it is already present.
  • The fail returns an error if targetField is already present, otherwise creates new field.
  • The insert results in no changes to the map if targetField is already present.

@TylerHelmuth
Copy link
Member

@aleksmaus do you have availability to bring this topic to the Collector SIG meeting? I think this function has enough edge cases to warrant a discussion there.

@aleksmaus
Copy link
Contributor Author

@aleksmaus do you have availability to bring this topic to the Collector SIG meeting? I think this function has enough edge cases to warrant a discussion there.

I have one meeting overlapping today at work, will see what I can do. If not this week then the next one.

Copy link
Contributor

github-actions bot commented Jun 3, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jun 3, 2024
@aleksmaus
Copy link
Contributor Author

After few discussions with the team, we are not going to pursue this change any further, closing this issue.
A couple of reasons to name:

  1. The conflict resolution (consistent with set) function is still up in discussions and current proposal is that it can be achieved with already available syntax.
  2. The function covers only a subset of functionality, and is only a convenience for the narrow scoped use cases. The more complete functionality can be achieved with already existing mechanisms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Community discussion needed enhancement New feature or request pkg/ottl Stale
Projects
None yet
Development

No branches or pull requests

4 participants