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.

Sign up for GitHub

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

Jump to bottom

filter plugin: include all transaction in group #5

Merged
merged 8 commits into from
Mar 10, 2023
Merged

filter plugin: include all transaction in group #5

merged 8 commits into from
Mar 10, 2023

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented Mar 10, 2023 •
edited
Loading

#1482
this PR adds a new filter flag omit-group-transactions. filter processor would return the matched txn and its grouped txns by default. when omit-group-transactions is set to false, only matched txns are returned.

Test

new unit tests

@shiqizng shiqizng added enhancement New feature or request Team Lamprey labels Mar 10, 2023
@shiqizng shiqizng self-assigned this Mar 10, 2023
@codecov
Copy link

codecov bot commented Mar 10, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@f5c2701). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master       #5   +/-   ##
=========================================
  Coverage          ?   67.23%           
=========================================
  Files             ?       32           
  Lines             ?     1819           
  Branches          ?        0           
=========================================
  Hits              ?     1223           
  Misses            ?      529           
  Partials          ?       67
Impacted Files Coverage Δ
...lugins/processors/filterprocessor/fields/filter.go 83.33% <100.00%> (ø)
...ins/processors/filterprocessor/filter_processor.go 80.28% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shiqizng
Copy link
Contributor Author 

BenchmarkProcess
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_true-8         	  462810	      2478 ns/op
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_true-8        	  154908	      7114 ns/op
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_true-8       	   23941	     49896 ns/op
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_false-8        	  264547	      4331 ns/op
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_false-8       	  105884	      9530 ns/op
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_false-8      	   22821	     54563 ns/op
PASS

winder
winder suggested changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

This can be done with a single iteration through the payset

conduit/plugins/processors/filterprocessor/config.go Show resolved Hide resolved
conduit/plugins/processors/filterprocessor/filter_processor.go Outdated Show resolved Hide resolved
@winder winder changed the title new-feature: filter transactions at the group level filter plugin: include all transaction in group Mar 10, 2023
@shiqizng
Copy link
Contributor Author

after simplifying the group txns filtering logic

BenchmarkProcess
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_true-8         	  455229	      2679 ns/op
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_true-8        	  128414	      8199 ns/op
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_true
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_true-8       	   22083	     54713 ns/op
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_0_omitGrouptxns_false-8        	  584884	      1903 ns/op
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_10_omitGrouptxns_false-8       	  170509	      6917 ns/op
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_false
BenchmarkProcess/inner_txn_count_100_omitGrouptxns_false-8      	   23548	     50624 ns/op
PASS

@shiqizng shiqizng requested a review from winder March 10, 2023 17:22
winder
winder approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Just a nit about the yaml file documentation. Looks good!

@shiqizng shiqizng requested a review from winder March 10, 2023 20:56
@shiqizng shiqizng merged commit 9c7f525 into master Mar 10, 2023
@shiqizng shiqizng deleted the shiqi/gtxns branch March 10, 2023 21:30
tzaffi referenced this pull request in tzaffi/conduit May 31, 2023
@shiqizng
filter plugin: include all transaction in group (#5)
ad9abad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winder winder winder approved these changes

@Eric-Warehime Eric-Warehime Eric-Warehime approved these changes

Assignees

@shiqizng shiqizng

Labels
enhancement New feature or request Team Lamprey
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shiqizng @winder @Eric-Warehime