fix(appsec): run the AppSec processor before the writer #3239
Conversation
vdeturckheim
added
the
changelog/no-changelog
|
@Kyle-Verhoog @brettlangdon I have updated the original PR:
PTAL. |
ddtrace/tracer.py
Outdated
| if appsec_enabled: | ||
| try: | ||
| from .appsec.processor import AppSecSpanProcessor | ||
| from .appsec._processor import AppSecSpanProcessor |
There was a problem hiding this comment.
I might have overlooked this in past PRs, but I was wondering if the tracer could implement some API to allow other components to interact with it. This current implementation seems to crate a tight bond between the tracer and AppSec (i.e. the tracer effectively knows about the .appsec module, even if there is a runtime check). This could be broken if the tracer allowed for some generic hooking mechanism, so that it can go on without knowing that .appsec exists. IMHO it's AppSec that should know about the tracer, and not the other way around. WDYT?
There was a problem hiding this comment.
I'd be +1 on that. At some point, doubling down on the gateway should bring us here.
However, we'll need to find a way to abstract writing on spans from these plugins.
Let's fix the current bug and move forward :)
There was a problem hiding this comment.
Yeah the idea was/is to expose some kind of API for adding/removing/specifying span processors we just haven't decided on a public API for this so we're keeping things internal to the tracer.
I think right now things are mostly contained in _initialize_span_processors so would be quite easily translated to whatever API we decide on.
|
@vdeturckheim this pull request is now in conflict 😩 |
|
@mergify update |
❌ Base branch update has failedmerge conflict between base and head Hey, I reacted but my real name is @Mergifyio |
…essor' into nicolas.vivet/hotfix-appsec-processor
ddtrace/tracer.py
Outdated
| if appsec_enabled: | ||
| try: | ||
| from .appsec.processor import AppSecSpanProcessor | ||
| from .appsec._processor import AppSecSpanProcessor |
There was a problem hiding this comment.
Yeah the idea was/is to expose some kind of API for adding/removing/specifying span processors we just haven't decided on a public API for this so we're keeping things internal to the tracer.
I think right now things are mostly contained in _initialize_span_processors so would be quite easily translated to whatever API we decide on.
vdeturckheim
dismissed stale reviews from brettlangdon and nizox
via
4fc594e
|
@Mergifyio backport 0.58 |
🟠 Waiting for conditions to match
|
* Run the AppSec processor before the writer * fix order issue - understand snapshot tests * fmt pass * try to enable snapshot for appsec * try to enable snapshot for appsec * try to enable snapshot for appsec * update snapshot * update snapshot * update snapshot * revert breaking change Co-authored-by: Nicolas Vivet <nicolas.vivet@datadoghq.com> (cherry picked from commit d89cb93)
✅ Backports have been created
|
* Run the AppSec processor before the writer * fix order issue - understand snapshot tests * fmt pass * try to enable snapshot for appsec * try to enable snapshot for appsec * try to enable snapshot for appsec * update snapshot * update snapshot * update snapshot * revert breaking change Co-authored-by: Nicolas Vivet <nicolas.vivet@datadoghq.com> (cherry picked from commit d89cb93) Co-authored-by: Vladimir de Turckheim <vdeturckheim@users.noreply.github.com>
This PR inserts the AppSec processor before the SpanAggregator processor. In fact, the writer runs at the end of this aggregator so any changes to the spans done after this processor won't be sent to the agent.
Replaces #3235 that was coming from a fork