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

chore: minor processor cleanup #3876

Merged
merged 7 commits into from
Nov 2, 2023
Merged

chore: minor processor cleanup #3876

merged 7 commits into from
Nov 2, 2023

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Sep 15, 2023

Description

Cleaned up some processor code.
Specifically the usage of 2 flags(trackingPlanEnabled and userTransformationEnabled) to decide the PU and inPU at various stages during the computation of metrics.
Also using stage literals from one package(utils/types) instead of two earlier(processor/transformer and utils/types).

Linear Ticket

< Replace with Linear Link >

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Attention: 134 lines in your changes are missing coverage. Please review.

Comparison is base (b80d273) 71.53% compared to head (988ff9a) 71.74%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3876      +/-   ##
==========================================
+ Coverage   71.53%   71.74%   +0.20%     
==========================================
  Files         373      373              
  Lines       54826    54891      +65     
==========================================
+ Hits        39219    39379     +160     
+ Misses      13272    13190      -82     
+ Partials     2335     2322      -13     
Files Coverage Δ
processor/trackingplan.go 95.45% <100.00%> (ø)
processor/transformer/transformer.go 90.79% <100.00%> (ø)
warehouse/api/http.go 78.94% <100.00%> (ø)
warehouse/app.go 89.59% <100.00%> (ø)
warehouse/integrations/testhelper/setup.go 99.68% <100.00%> (ø)
warehouse/internal/model/upload.go 100.00% <ø> (ø)
warehouse/integrations/testhelper/verify.go 89.64% <91.66%> (ø)
warehouse/internal/model/source.go 91.66% <91.66%> (ø)
warehouse/slave/worker.go 81.74% <76.19%> (-0.88%) ⬇️
warehouse/internal/repo/table_upload.go 91.69% <90.69%> (+0.13%) ⬆️
... and 4 more

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atzoum
Copy link
Contributor

atzoum commented Sep 20, 2023

@Sidddddarth how do we verify no regression has been introduced?

Copy link
Contributor

@atzoum atzoum left a comment

Choose a reason for hiding this comment

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

  • transformer.EventFilterStage is unused
  • transformer.TrackingPlanValidationStage is being used by processor_test.go

It might be best to unexport stage constants, since they appear to be for internal use only:

  • userTransformerStage
  • destTransformerStage
  • trackingPlanValidationStage

@Sidddddarth
Copy link
Member Author

@Sidddddarth how do we verify no regression has been introduced?

adding some coverage here

@atzoum
Copy link
Contributor

atzoum commented Oct 12, 2023

Please add a description of the changes, helpful to the review process

@Sidddddarth Sidddddarth force-pushed the chore.ProcessorCleanup branch 2 times, most recently from 139d017 to 0d2365d Compare October 13, 2023 07:09
@atzoum
Copy link
Contributor

atzoum commented Oct 13, 2023

@Sidddddarth how do we verify no regression has been introduced?

adding some coverage here

why is it on a separate PR?

@Sidddddarth
Copy link
Member Author

@Sidddddarth how do we verify no regression has been introduced?

adding some coverage here

why is it on a separate PR?

That started when there were no tests for reporting at all. Considering we have some now, they're of no use anymore.
And I did it in another PR so we could have assertions in place before I change code that is heavily involved in the pipeline.
Closing that PR now.

@Sidddddarth Sidddddarth force-pushed the chore.ProcessorCleanup branch 4 times, most recently from 2d29d1e to 0746bbf Compare October 19, 2023 09:14
@Sidddddarth Sidddddarth force-pushed the chore.ProcessorCleanup branch 2 times, most recently from 3824cd2 to ae1e5bd Compare October 26, 2023 08:53
@Sidddddarth Sidddddarth force-pushed the chore.ProcessorCleanup branch 2 times, most recently from 8ac08f0 to 99bccdb Compare November 1, 2023 01:19
processor/processor_test.go Outdated Show resolved Hide resolved
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.

3 participants