Skip to content

Remove inline RISC job #10826

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

Merged
vrajmohan merged 1 commit into from
Jun 20, 2024
Merged

Remove inline RISC job #10826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions app/jobs/risc_delivery_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def perform(
'Content-Type' => 'application/secevent+jwt',
) do |req|
req.options.context = {
service_name: inline? ? 'risc_http_push_direct' : 'risc_http_push_async',
service_name: 'risc_http_push_async',
}
end
end
Expand All @@ -58,7 +58,7 @@ def perform(
user:,
)
rescue *NETWORK_ERRORS => err
raise err if self.executions < 2 && !inline?
raise err if self.executions < 2

track_event(
error: err.message,
Expand All @@ -68,7 +68,7 @@ def perform(
user:,
)
rescue RedisRateLimiter::LimitError => err
raise err if self.executions < 10 && !inline?
raise err if self.executions < 10

track_event(
error: err.message,
Expand Down Expand Up @@ -102,18 +102,13 @@ def faraday
end
end

def inline?
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jun 18, 2024

Choose a reason for hiding this comment

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

if we're following through with this we should also update the logic in http_push.rb and remove the risc_notifications_active_job_enabled feature flag as well

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Jun 18, 2024

Choose a reason for hiding this comment

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

Right, I saw that when the tests failed on CI. Looking into it.

queue_adapter.is_a?(ActiveJob::QueueAdapters::InlineAdapter)
end

def track_event(event_type:, issuer:, success:, user:, error: nil, status: nil)
analytics(user).risc_security_event_pushed(
client_id: issuer,
error:,
event_type:,
status:,
success:,
transport: inline? ? 'direct' : 'async',
)
end

Expand Down
3 changes: 0 additions & 3 deletions app/services/analytics_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4919,14 +4919,12 @@ def return_to_sp_failure_to_proof(redirect_url:, flow: nil, step: nil, location:
# @param [String] client_id
# @param [String] event_type
# @param [Boolean] success
# @param ['async'|'direct'] transport
# @param [Integer] status
# @param [String] error
def risc_security_event_pushed(
client_id:,
event_type:,
success:,
transport:,
status: nil,
error: nil,
**extra
Expand All @@ -4938,7 +4936,6 @@ def risc_security_event_pushed(
event_type:,
status:,
success:,
transport:,
**extra,
)
end
Expand Down
10 changes: 2 additions & 8 deletions app/services/push_notification/http_push.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,12 @@ def url_options
def deliver_one(service_provider)
deliver_local(service_provider) if IdentityConfig.store.risc_notifications_local_enabled

job_arguments = {
RiscDeliveryJob.perform_later(
push_notification_url: service_provider.push_notification_url,
jwt: jwt(service_provider),
event_type: event.event_type,
issuer: service_provider.issuer,
}

if IdentityConfig.store.risc_notifications_active_job_enabled
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jun 20, 2024

Choose a reason for hiding this comment

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

this was the last usage of this feature flag, so now let's fully remove it everywhere by taking it out of application.yml.default and identity_config.rb

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Jun 20, 2024

Choose a reason for hiding this comment

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

This was what I intended with my commit comment -> How -> 3rd bullet. Did you mean something else?

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jun 20, 2024

Choose a reason for hiding this comment

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

Feature flags are safe to remove in the codebase in the same PR that stop using them, so no need for a second PR

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Jun 20, 2024

Choose a reason for hiding this comment

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

Got it, I should have remembered that.

RiscDeliveryJob.perform_later(**job_arguments)
else
RiscDeliveryJob.perform_now(**job_arguments)
end
)
end

def deliver_local(service_provider)
Expand Down
1 change: 0 additions & 1 deletion config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ reset_password_email_max_attempts: 20
reset_password_email_window_in_minutes: 60
reset_password_on_auth_fraud_event: true
risc_notifications_local_enabled: false
risc_notifications_active_job_enabled: false
risc_notifications_rate_limit_interval: 60
risc_notifications_rate_limit_max_requests: 1_000
risc_notifications_rate_limit_overrides: '{"https://example.com/push":{"interval":120,"max_requests":10000}}'
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,6 @@ def self.store
config.add(:reset_password_email_max_attempts, type: :integer)
config.add(:reset_password_email_window_in_minutes, type: :integer)
config.add(:reset_password_on_auth_fraud_event, type: :boolean)
config.add(:risc_notifications_active_job_enabled, type: :boolean)
config.add(:risc_notifications_local_enabled, type: :boolean)
config.add(:risc_notifications_rate_limit_interval, type: :integer)
config.add(:risc_notifications_rate_limit_max_requests, type: :integer)
Expand Down
Loading