-
Notifications
You must be signed in to change notification settings - Fork 453
fix(appsec): report all tags on the service entry span instead of the local root span #14210
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.
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
fix(appsec): report all tags on the service entry span instead of the local root span #14210
Conversation
cf74a41
to
4453c1f
Compare
|
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 273 ± 3 ms. The average import time from base is: 275 ± 2 ms. The import time difference between this PR and base is: -2.3 ± 0.1 ms. Import time breakdownThe following import paths have shrunk:
|
4453c1f
to
95a54a8
Compare
Performance SLOsCandidate: florentinl/APPSEC-58532/report-tags-on-service-entry-span (a048425) 🔵 No Baseline Data (24 suites)🔵 coreapiscenario - 12/12 (2 unstable)🔵 No baseline data available for this suite
|
bd9d326
to
e7f2dde
Compare
e7f2dde
to
050c4b5
Compare
d548fb5
to
c14225e
Compare
709bc4c
to
2fc795b
Compare
2fc795b
to
784ccf0
Compare
Great Work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Span._service_entry_span_value
is an internal property so my comments/concerns are non-blocking. We can iterate on this design in future PRs.
898ffbc
to
a048425
Compare
Motivation
In specific cases where we have an inferred span (belonging to an inferred service) as the parent of the framework instrumented web request (the 'web' span), Appsec is enabled on the inferred service instead of the current service.
This is because we always use set tags and metrics on the root span using the
span._local_root
helper.This leads to an incorrect reporting of the appsec enablement status, and security signals showing up on spans of a different service.
Note:
Changes
Add a helper on spans to retrieve the top level span of a given service
Modify the appsec logic to always report on the service entry span and add a failsafe to query the entry span of the current span or current root span to get a handle on a span in any situation.
Modify the tests to look for the entry span instead of the root span
Notes:
_DD_IAST_USE_ROOT_SPAN
flag. This setting also needs additional manual handling inreport_stack
as reporting to root span is not the default anymore.Checklist
Reviewer Checklist