Conversation
Typing analysisNote: Ignored files are excluded from the next sections. Untyped methodsThis PR introduces 1 partially typed method, and clears 1 partially typed method. It increases the percentage of typed methods from 61.55% to 61.58% (+0.03%). Partially typed methods (+1-1)❌ Introduced:Untyped other declarationsThis PR introduces 2 untyped other declarations, and clears 2 untyped other declarations. It decreases the percentage of typed other declarations from 77.64% to 77.56% (-0.08%). Untyped other declarations (+2-2)❌ Introduced:If you believe a method or an attribute is rightfully untyped or partially typed, you can add |
|
✨ Fix all issues with BitsAI or with Cursor
|
1880047 to
a1f38df
Compare
BenchmarksBenchmark execution time: 2026-03-20 15:59:12 Comparing candidate commit 7248786 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 0 unstable metrics.
|
| @@ -63,7 +63,7 @@ def record_successful_signin(context, resource) | |||
| # and because of that we will trigger an additional event even | |||
| # if it was already done via the SDK | |||
| AppSec::Instrumentation.gateway.push( | |||
| 'identity.set_user', AppSec::Instrumentation::Gateway::User.new(id, login) | |||
| 'identity.set_user', {id: id, login: login, framework: 'devise', event_type: 'login_success'} | |||
There was a problem hiding this comment.
I think event_type is a name for the event we are pushing, like identity.login_failure we are doing next.
I find it cool to scope them with identity.<event> pattern, but passing event name in event_type field when we have an event name as a first argument is not a good choice.
There was a problem hiding this comment.
good point. I changed the events we are sending - you can see the full list in the PR description
| def report_missing_user_telemetry(user_info, event_type) | ||
| tags = {framework: user_info[:framework], event_type: event_type} | ||
|
|
||
| missing_login = user_info[:login].nil? | ||
| missing_id = user_info[:id].nil? | ||
|
|
||
| if missing_login && event_type != 'authenticated_request' | ||
| AppSec.telemetry.inc( | ||
| Ext::TELEMETRY_METRICS_NAMESPACE, | ||
| 'instrum.user_auth.missing_user_login', | ||
| 1, | ||
| tags: tags, | ||
| ) | ||
| end |
There was a problem hiding this comment.
I think this is a lot to do for report method, feels like the whole logic of what to report when got offloaded to this method instead of a watcher. I think this is a wrong responsibility split.
There was a problem hiding this comment.
now with separate events it is simpler - it only checks for id and login presence
| allow(Datadog::AppSec::Instrumentation).to receive(:gateway).and_return(gateway) | ||
| Datadog::AppSec::Monitor::Gateway::TelemetryWatcher.watch | ||
|
|
||
| allow(Datadog::AppSec.telemetry).to receive(:inc).and_call_original |
dc19805 to
297f77a
Compare
| IDENTITY_EVENTS = %w[ | ||
| identity.sdk.set_user | ||
| identity.sdk.login_success | ||
| identity.sdk.login_failure | ||
| identity.devise.login_success | ||
| identity.devise.login_failure | ||
| identity.devise.signup | ||
| identity.devise.authenticated_request | ||
| ].freeze | ||
|
|
||
| LOGIN_EVENTS = { | ||
| 'identity.sdk.login_success' => 'server.business_logic.users.login.success', | ||
| 'identity.sdk.login_failure' => 'server.business_logic.users.login.failure', | ||
| 'identity.devise.login_success' => 'server.business_logic.users.login.success', | ||
| 'identity.devise.login_failure' => 'server.business_logic.users.login.failure', | ||
| }.freeze |
There was a problem hiding this comment.
this duplication is an intermediate step, before we add pattern-based subscription (e.g. identity.*.login_success, or identity.*.*)
What does this PR do?
identity.<framework>.<action>(e.g.identity.devise.login_success,identity.sdk.set_user).appsec.events.user_lifecyclegateway event. Its business logic WAF check is now handled by a newwatch_user_login_eventwatcher on theidentity.*login events directly.AppSec::Monitor::Gateway::TelemetryWatcherthat reportsinstrum.user_auth.missing_user_loginandinstrum.user_auth.missing_user_idtelemetry metrics when user auth events have incomplete data.Datadog::AppSec::Instrumentation::Gateway::Argument::Userand replaces it with a plain hash.Motivation:
Simplify the AppSec gateway event architecture. We also want to know when we can't extract user login or id during SDK calls and Devise instrumentation, which the new TelemetryWatcher provides.
Change log entry
No. This is an internal change.
Additional Notes:
New event names:
identity.sdk.set_user— fromKit::Identity.set_userand V2 SDKidentity.sdk.login_success— from V2 SDK login success (business logic WAF)identity.sdk.login_failure— from V2 SDK login failureidentity.devise.login_success/login_failure/signup/authenticated_request— from Devise auto-instrumentationAPPSEC-60123
How to test the change?
CI and manual testing.