Make appsec work without tracing #3645
Conversation
github-actions
bot
added
appsec
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3645 +/- ##
==========================================
- Coverage 98.10% 98.10% -0.01%
==========================================
Files 1225 1232 +7
Lines 72464 72556 +92
Branches 3439 3447 +8
==========================================
+ Hits 71094 71184 +90
- Misses 1370 1372 +2 ☔ View full report in Codecov by Sentry. |
| @@ -13,24 +13,32 @@ module Tracking | |||
| SIGNUP_EVENT = 'users.signup' | |||
|
|
|||
| def self.track_login_success(trace, span, user_id:, **others) | |||
| return if trace.nil? && span.nil? | |||
There was a problem hiding this comment.
Does it make sense to use || instead of && ?
There was a problem hiding this comment.
Oh, right, I reversed things: both trace and span must be non-nil to continue, so if either is nil we must return.
lloeki
force-pushed
the
make-appsec-devise-work-without-tracing
branch
from
db9dd09
to
6887455
Compare
lloeki
force-pushed
the
make-appsec-devise-work-without-tracing
branch
from
6887455
to
0a9d812
Compare
| def self.track_login_success(trace, span, user_id:, **others) | ||
| return if trace.nil? || span.nil? | ||
|
|
||
| track(LOGIN_SUCCESS_EVENT, trace, span, **others) | ||
|
|
||
| Kit::Identity.set_user(trace, span, id: user_id.to_s, **others) if user_id | ||
| end | ||
|
|
||
| def self.track_login_failure(trace, span, user_id:, user_exists:, **others) | ||
| return if trace.nil? || span.nil? | ||
|
|
||
| track(LOGIN_FAILURE_EVENT, trace, span, **others) | ||
|
|
||
| span.set_tag('appsec.events.users.login.failure.usr.id', user_id) if user_id | ||
| span.set_tag('appsec.events.users.login.failure.usr.exists', user_exists) | ||
| end | ||
|
|
||
| def self.track_signup(trace, span, user_id:, **others) | ||
| return if trace.nil? || span.nil? | ||
|
|
||
| track(SIGNUP_EVENT, trace, span, **others) | ||
| Kit::Identity.set_user(trace, id: user_id.to_s, **others) if user_id | ||
| end | ||
|
|
||
| def self.track(event, trace, span, **others) | ||
| return if trace.nil? || span.nil? | ||
|
|
||
| span.set_tag("appsec.events.#{event}.track", 'true') | ||
| span.set_tag("_dd.appsec.events.#{event}.auto.mode", Datadog.configuration.appsec.track_user_events.mode) | ||
|
|
There was a problem hiding this comment.
Would be cool to add type signatures for this module -- I think they would've caught this bug :)
There was a problem hiding this comment.
That is very true @ivoanjo!
@p-datadog This needs something more of an integration test such as is done in there with dd-trace-rb/spec/datadog/appsec/contrib/rack/integration_test_spec.rb Lines 694 to 705 in c989b96 |
|
Merging this as is, I took note to add typing + an integration test later. |
2.0 Upgrade Guide notes
What does this PR do?
Make appsec work without tracing
Motivation:
Fixes #3564
Additional Notes:
Probably will need to be backported.
How to test the change?
Unsure? Have a question? Request a review!