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

Allow transform_generic to keep existing fields #156

Merged
merged 7 commits into from
Apr 5, 2022

Conversation

KalmanMeth
Copy link
Collaborator

Addresses issue #151

@KalmanMeth KalmanMeth requested review from jotak, eranra and ronensc March 27, 2022 11:48
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #156 (e4d9ee4) into main (485fa3c) will increase coverage by 0.47%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   58.47%   58.95%   +0.47%     
==========================================
  Files          51       54       +3     
  Lines        2950     3055     +105     
==========================================
+ Hits         1725     1801      +76     
- Misses       1113     1134      +21     
- Partials      112      120       +8     
Flag Coverage Δ
unittests 58.95% <76.47%> (+0.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/api/enum.go 68.00% <ø> (ø)
pkg/api/transform_generic.go 0.00% <0.00%> (ø)
pkg/confgen/flowlogs2metrics_config.go 0.00% <0.00%> (ø)
pkg/pipeline/transform/transform_generic.go 100.00% <100.00%> (ø)
pkg/pipeline/utils/exit.go 82.60% <0.00%> (-17.40%) ⬇️
pkg/config/config.go 50.00% <0.00%> (ø)
pkg/pipeline/write/write_stdout.go 100.00% <0.00%> (ø)
pkg/pipeline/ingest/ingest_grpc.go 61.53% <0.00%> (ø)
pkg/pipeline/decode/decode_protobuf.go 74.54% <0.00%> (ø)
pkg/pipeline/pipeline_builder.go 59.92% <0.00%> (+1.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 485fa3c...e4d9ee4. Read the comment docs.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@KalmanMeth KalmanMeth force-pushed the trasnform-generic-ext branch from e5f96a5 to f625bf7 Compare March 28, 2022 13:36
@KalmanMeth
Copy link
Collaborator Author

The default is now to include all previous key/values and to enrich them. To get only the newly specified keys, we now have policy: replace_keys as a parameter in the yaml.

@KalmanMeth
Copy link
Collaborator Author

addresses issue #151

Copy link
Collaborator

@eranra eranra left a comment

Choose a reason for hiding this comment

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

Comment on lines +255 to +256
To include the original keys and values in addition to those specified in the `rules`,
specify `policy: preserve_original_keys`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen if an original key conflicts with a key specified in rules? (i.e. they have the same name)
Is it an error? does the original key prevail? or the new key?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new key overwrites the old value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think its worth noting in the docs?

output := make([]config.GenericMap, 0)
for _, entry := range input {
outputEntry := make(config.GenericMap)
for _, transformRule := range g.Rules {
outputEntry := entry
Copy link
Collaborator

Choose a reason for hiding this comment

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

if g.policy != "replace_keys" then this code is modifying the input slice. This might be a problem if the same slice is passed to a parallel transform stage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. Copied map entries individually.

outputEntry := make(config.GenericMap)
for _, transformRule := range g.Rules {
outputEntry := entry
if g.policy == "replace_keys" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to panic on an invalid policy value? I mean, if the user writes replace-keys with a dash instead of an underline, we will treat it as preserve_original_keys

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added panic for illegal policy

@KalmanMeth KalmanMeth force-pushed the trasnform-generic-ext branch from e4d9ee4 to 7c80873 Compare April 4, 2022 13:02
@KalmanMeth KalmanMeth merged commit 3d0abe3 into netobserv:main Apr 5, 2022
@KalmanMeth KalmanMeth deleted the trasnform-generic-ext branch April 5, 2022 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants