Skip to content

Commit 3b495c5

Browse files
fix(asm): fix manual keep for user event (#7296)
Partial backport of #6633 to fix user events being dropped by the tracer ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [ ] Title is accurate. - [ ] No unnecessary changes are introduced. - [ ] Description motivates each change. - [ ] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [ ] Testing strategy adequately addresses listed risk(s). - [ ] Change is maintainable (easy to change, telemetry, documentation). - [ ] Release note makes sense to a user of the library. - [ ] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [ ] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) - [ ] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. - [ ] This PR doesn't touch any of that.
1 parent b1c0bba commit 3b495c5

File tree

3 files changed

+11
-7
lines changed

3 files changed

+11
-7
lines changed

ddtrace/appsec/trace_utils.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ def _track_user_login_common(
6161
if name:
6262
span.set_tag_str("%s.username" % tag_prefix, name)
6363

64-
span.set_tag_str(constants.MANUAL_KEEP_KEY, "true")
64+
span.set_tag(constants.MANUAL_KEEP_KEY)
6565
return span
6666
else:
6767
log.warning(
@@ -133,7 +133,7 @@ def track_user_signup_event(tracer, user_id, success, login_events_mode=LOGIN_EV
133133
success_str = "true" if success else "false"
134134
span.set_tag_str(APPSEC.USER_SIGNUP_EVENT, success_str)
135135
span.set_tag_str(user.ID, user_id)
136-
span.set_tag_str(constants.MANUAL_KEEP_KEY, "true")
136+
span.set_tag(constants.MANUAL_KEEP_KEY)
137137

138138
# This is used to mark if the call was done from the SDK of the automatic login events
139139
if login_events_mode == LOGIN_EVENTS_MODE.SDK:
@@ -182,7 +182,7 @@ def track_custom_event(tracer, event_name, metadata):
182182

183183
for k, v in six.iteritems(metadata):
184184
span.set_tag_str("%s.%s.%s" % (APPSEC.CUSTOM_EVENT_PREFIX, event_name, k), str(v))
185-
span.set_tag_str(constants.MANUAL_KEEP_KEY, "true")
185+
span.set_tag(constants.MANUAL_KEEP_KEY)
186186

187187

188188
def should_block_user(tracer, userid): # type: (Tracer, str) -> bool
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
ASM: This fix resolves an issue where user events spans could be sampled erroneously.

tests/appsec/test_appsec_trace_utils.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ def test_track_user_login_event_success_without_metadata(self):
5151
assert root_span.get_tag("%s.sdk" % success_prefix) == "true"
5252
assert not root_span.get_tag("%s.auto.mode" % success_prefix)
5353
assert not root_span.get_tag("%s.track" % failure_prefix)
54-
assert root_span.get_tag(constants.MANUAL_KEEP_KEY) == "true"
54+
assert root_span.context.sampling_priority == constants.USER_KEEP
5555
# set_user tags
5656
assert root_span.get_tag(user.ID) == "1234"
5757
assert root_span.get_tag(user.NAME) == "John"
@@ -109,7 +109,7 @@ def test_track_user_login_event_success_auto_mode_safe(self):
109109

110110
root_span = self.tracer.current_root_span()
111111
success_prefix = "%s.success" % APPSEC.USER_LOGIN_EVENT_PREFIX
112-
assert root_span.get_tag("%s.track" % success_prefix) == "true"
112+
assert root_span.context.sampling_priority == constants.USER_KEEP
113113
assert not root_span.get_tag("%s.sdk" % success_prefix)
114114
assert root_span.get_tag("%s.auto.mode" % success_prefix) == str(LOGIN_EVENTS_MODE.SAFE)
115115

@@ -142,7 +142,7 @@ def test_track_user_login_event_success_with_metadata(self):
142142
assert root_span.get_tag("%s.sdk" % success_prefix) == "true"
143143
assert not root_span.get_tag("%s.auto.mode" % success_prefix)
144144
assert root_span.get_tag("%s.foo" % success_prefix) == "bar"
145-
assert root_span.get_tag(constants.MANUAL_KEEP_KEY) == "true"
145+
assert root_span.context.sampling_priority == constants.USER_KEEP
146146
# set_user tags
147147
assert root_span.get_tag(user.ID) == "1234"
148148
assert not root_span.get_tag(user.NAME)
@@ -173,7 +173,7 @@ def test_track_user_login_event_failure_user_exists(self):
173173
assert root_span.get_tag("%s.%s" % (failure_prefix, user.ID)) == "1234"
174174
assert root_span.get_tag("%s.%s" % (failure_prefix, user.EXISTS)) == "true"
175175
assert root_span.get_tag("%s.foo" % failure_prefix) == "bar"
176-
assert root_span.get_tag(constants.MANUAL_KEEP_KEY) == "true"
176+
assert root_span.context.sampling_priority == constants.USER_KEEP
177177
# set_user tags: shouldn't have been called
178178
assert not root_span.get_tag(user.ID)
179179
assert not root_span.get_tag(user.NAME)

0 commit comments

Comments
 (0)