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

New processor: copy_fields #11303

Merged
merged 13 commits into from
Mar 22, 2019
Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Mar 19, 2019

A new processor is introduced as part of support for keeping orignal messages. Options and naming follows the convention of other processors.

copy_fields

This processor copies one field to another. Example configuration is below:

processors:
- copy_fields:
    fields:
      - from: message
        to: event.original
    fail_on_error: false
    ignore_missing: true

Blocks #11297

@kvch kvch requested review from a team as code owners March 19, 2019 10:23
@kvch kvch requested review from ruflin and ph and removed request for a team March 19, 2019 10:23
@kvch kvch force-pushed the feature-libbeat-new-copy-processor branch from a65f82e to 30cfee1 Compare March 19, 2019 10:27
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Needs a changelog entry.

libbeat/processors/actions/copy_fields.go Outdated Show resolved Hide resolved
libbeat/processors/actions/copy_fields.go Show resolved Hide resolved
libbeat/processors/actions/copy_fields.go Outdated Show resolved Hide resolved
@kvch kvch force-pushed the feature-libbeat-new-copy-processor branch from c7fc1c2 to fe47eba Compare March 19, 2019 12:50
libbeat/processors/actions/copy_fields.go Outdated Show resolved Hide resolved
# - from: message
# to: message_copied
# fail_on_error: false
# ignore_missing: true
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this is exactly the opposite to what we have in the defaults in the code for fail_on_error and ignore_missing. Is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Thanks for finding it.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is still missing?

},
"copy number from fieldname with dot to dotted message.copied": {
FromTo: fromTo{
From: "message.original",
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you copy message.original to message? Will it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set message.original as a single key containing a dot, so it does not lead to an error when someone wants to manipulate the submaps under message. It is a different key.

If the test input was different e.g. the key message was already present

Input: common.MapStr{
    "message": common.MapStr{
        "original": 42,
    },
}

It would fail and tell the user to drop the field message first, before attempting to copy stuff.

This is a more realistic test case, so I am adding it also.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, good to know. Could you add a test case for both and one of them would fail. The reason I'm asking also for the failing one is because in case we change the behaviour of the above in the future, we will know that it has an impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test with the description "copy number from hierarchical message.original to top level message which fails" will fail if the behaviour changes, as the new field would show up in the output. I have also added a new test case which demonstrates what happens when the key contains a dot and is present as a top level key in the fields. If MapStr is changed one of the tests will fail.

libbeat/processors/actions/rename.go Outdated Show resolved Hide resolved
libbeat/processors/actions/rename_test.go Outdated Show resolved Hide resolved
libbeat/processors/actions/rename_test.go Outdated Show resolved Hide resolved
@kvch
Copy link
Contributor Author

kvch commented Mar 21, 2019

Failing test in Travis is unrelated.

err := f.copyField(field.From, field.To, event.Fields)
if err != nil && f.config.FailOnError {
errMsg := fmt.Errorf("Failed to copy fields in copy_fields processor: %s", err)
logp.Debug("copy_fields", errMsg.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a selector for logger? I like what @andrewkroh's did in the script processor, see

Tag string `config:"tag"` // Processor ID for debug and metrics.

And

logName = "processor.javascript"

Copy link
Contributor Author

@kvch kvch Mar 22, 2019

Choose a reason for hiding this comment

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

TBH in case of this simple processor, I don't see the added value of letting a user set a debug selector. I doubt that users would need to filter for a specific copy processor in their logs, or if they have to, I think messages let then identify the processor easily.

If we want to let user set a selector, it would be much better to let them configure it for all processors. There are other processors which would benefit more from having such setting than this one. What if I open an issue and implement the functionality for all processors in a follow up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK with a followup PR.

@kvch kvch force-pushed the feature-libbeat-new-copy-processor branch from f4579e3 to 4d3c2f5 Compare March 22, 2019 09:03
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

@kvch small issue with the changelog but LGTM

@@ -257,6 +257,9 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Add ip fields to default_field in Elasticsearch template. {pull}11035[11035]
- Gracefully shut down on SIGHUP {pull}10704[10704]
- Add `script` processor that supports using Javascript to process events. {pull}10850[10850] {pull}11260[11260]
- Add `script` processor that supports using Javascript to process events. {pull}10850[10850]
- New processor: `copy_fields`. {pull}11303[11303]
- Add error.message to events when `fail_on_error` is set in `rename` and `copy_fields` processors. {pull}11303[11303]
Copy link
Contributor

Choose a reason for hiding this comment

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

problems with the rebase?

@kvch kvch merged commit 4b9f945 into elastic:master Mar 22, 2019
@andrewkroh
Copy link
Member

@kvch Can you please make sure this new processor gets added to the documentation.

DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
A new processor is introduced as part of support for keeping orignal messages. Options and naming follows the convention of other processors.

### `copy_fields`

This processor copies one field to another. Example configuration is below:

```yaml
processors:
- copy_fields:
    fields:
      - from: message
        to: event.original
    fail_on_error: false
    ignore_missing: true
```
@cwurm
Copy link
Contributor

cwurm commented Sep 17, 2019

@kvch can you add the docs when you have a minute? I was just looking for this functionality and couldn't find it.

@zez3
Copy link

zez3 commented Sep 12, 2022

@kvch or @ruflin
Does this copy_fields processor actually append to an existing field(value) as array or fails on any error?

@dedemorton
It's not clear from the documentation if that happens or not.
Can someone plz fix that? Thx

@dedemorton
Copy link
Contributor

@zez3 Please open an issue to report Beats documentation problems. Comments on old PRs are likely to get lost. Thanks!

@dedemorton
Copy link
Contributor

@zez3 Also...forgot to mention that you can ask questions and search for answers in the discuss forum.

@zez3
Copy link

zez3 commented Sep 14, 2022

@dedemorton
I did searched the discussion forum but most of the proposed workarounds for append@beat level are using JS script or ES pipelines.
Ideally the copy_fields documentation should state that this processor does not append anything and that it actuality replaces the value(s).

I've opened a new 33088 Issue

@dedemorton
Copy link
Contributor

Thanks for creating the issue! I've added it to our project queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants