LG-13386 Add SP costs in the ProgressiveProofer class#10767
Merged
Conversation
matthinz
approved these changes
Jun 5, 2024
zachmargolis
approved these changes
Jun 5, 2024
Base automatically changed from
jmhooper-remove-use-ivars-in-progressive-proofer
to
main
June 5, 2024 19:58
jmhooper
force-pushed
the
jmhooper-add-sp-cost-in-prog-proofer
branch
from
d9ee795 to
4f4a1c3
Compare
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In #10743 we stopped writing the costs iin the controller if the result hash indicates that costs were alreay written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in #10743 are fully deployed. [skip changelog]
jmhooper
force-pushed
the
jmhooper-add-sp-cost-in-prog-proofer
branch
from
a07554d to
31f7acc
Compare
jmhooper
added a commit
that referenced
this pull request
Jun 11, 2024
In #10767 we changed the `ProgressiveProofer` class to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in the `VerifyInfoConcern` to deal with the 50/50 state. When the changes in #10767 are fully deployed this can be merged to clean up dead code. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 13, 2024
In #10767 we changed the `ProgressiveProofer` class to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in the `VerifyInfoConcern` to deal with the 50/50 state. When the changes in #10767 are fully deployed this can be merged to clean up dead code. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 13, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
brandemix
pushed a commit
to brandemix/18F-identity-idp
that referenced
this pull request
Jun 17, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In 18F#10743 we stopped writing the costs in the controller if the result hash indicates that costs were already written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in 18F#10743 are fully deployed. [skip changelog]
brandemix
pushed a commit
to brandemix/18F-identity-idp
that referenced
this pull request
Jun 17, 2024
…10786) In 18F#10767 we changed the `ProgressiveProofer` class to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in the `VerifyInfoConcern` to deal with the 50/50 state. When the changes in 18F#10767 are fully deployed this can be merged to clean up dead code. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency.
In #10743 we stopped writing the costs in the controller if the result hash indicates that costs were already written. This commit does the work of writing costs in the
ProgressiveProoferand includes that they were written in the result.The next step will be removing the logic to write costs in the controller
This commit can only be safely deployed when the changes in #10743 are fully deployed.