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

Fix isolation of modules when merging local and global field settings. #5808

Merged
merged 4 commits into from
Dec 6, 2017

Conversation

urso
Copy link

@urso urso commented Dec 5, 2017

Resolves: #5795

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

urso added 4 commits December 5, 2017 15:08
The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.
@urso urso force-pushed the bug/field-overwrites branch from 899026e to 69eb2c6 Compare December 5, 2017 14:08
@urso urso added bug libbeat needs_backport PR is waiting to be backported to other branches. labels Dec 5, 2017
@urso urso requested review from ruflin and andrewkroh December 5, 2017 14:09
@@ -68,7 +68,8 @@ type pipelineProcessors struct {

processors beat.Processor

disabled bool // disabled is set if outputs have been disabled via CLI
disabled bool // disabled is set if outputs have been disabled via CLI
alwaysCopy bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this variable get its value?

Copy link
Author

Choose a reason for hiding this comment

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

I introduced it for the unit tests enforce the copy of fields. This way, when adding more tests we can see if copy vs. non-copy of fields makes a difference.

Copy link
Contributor

@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.

WDYT about adding a system test to verify the change?

@urso
Copy link
Author

urso commented Dec 5, 2017

WDYT about adding a system test to verify the change?

I reproduced + verified the issue via unit tests. Really need an additional system test here?

@ruflin
Copy link
Contributor

ruflin commented Dec 6, 2017

@urso Not required, only nice to have. Glad we have the unit tests.

Copy link
Contributor

@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.

LGTM. One minor question.

@@ -39,7 +39,9 @@ type processorFn struct {
// 8. (P) pipeline processors list
// 9. (P) (if publish/debug enabled) log event
// 10. (P) (if output disabled) dropEvent
func (p *Pipeline) newProcessorPipeline(
func newProcessorPipeline(
info beat.Info,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you decide to pass info instead of using it as variable as before?

Copy link
Author

Choose a reason for hiding this comment

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

The info is used for the debug print. It used to be part of the pipeline variable. I removed the method and created a constructor passing only those information required to build the processor pipeline. The decoupling simplified the construction of the processor pipeline in the unit tests.

@ruflin ruflin merged commit 08eb2f5 into elastic:master Dec 6, 2017
@ruflin
Copy link
Contributor

ruflin commented Dec 6, 2017

@urso Can you take care of the backports?

@urso urso removed the needs_backport PR is waiting to be backported to other branches. label Dec 6, 2017
@ruflin
Copy link
Contributor

ruflin commented Dec 7, 2017

@urso I think we also need to backport this to 6.0?

urso pushed a commit to urso/beats that referenced this pull request Dec 7, 2017
elastic#5808)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit 08eb2f5)
urso pushed a commit to urso/beats that referenced this pull request Dec 7, 2017
elastic#5808)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit 08eb2f5)
ruflin pushed a commit that referenced this pull request Dec 7, 2017
#5808) (#5836)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit 08eb2f5)
ruflin pushed a commit that referenced this pull request Dec 8, 2017
#5808) (#5825)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit 08eb2f5)
@urso urso deleted the bug/field-overwrites branch February 19, 2019 18:54
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
elastic#5808) (elastic#5825)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit a16dc30)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
elastic#5808) (elastic#5836)

The user global and local fields configurations must be merged
on pipeline setup. The DeepUpdate method did only keep references to the
globally shared fields, such that local fields settings do overwrite
the shared global state. If more then one beat.Client instance did set
local fields, the global state (and all fields added to an event) will
be given by the last client being instantiated.

The fix Clones the global fields before merging, such that the
beat.Client fields operate fully in isolation.

(cherry picked from commit a16dc30)
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.

Fields merging in 6.0 differs from 5.x
3 participants