contrib/go-chi/chi: add appsec monitoring #1130
Conversation
Julio-Guerra
force-pushed
the
julio.guerra/appsec-chi
branch
from
bc74f85 to
4e03b0f
Compare
| appsec.Start() | ||
| defer appsec.Stop() | ||
|
|
||
| if !appsec.Enabled() { | ||
| t.Skip("appsec disabled") | ||
| } |
There was a problem hiding this comment.
For now, this API is only used internally, but consider that this form may be confusing if and when it becomes public API. I mean, this is what happens:
- AppSec gets started on line 309
- Line 312 checks if it's enabled and skips the test, even though it was never stopped.
I understand the fact that there is an explanation for this, and that Start might not actually start AppSec, but nevertheless it's confusing because a function like appsec.Start which returns no error is assumed to succeed. Perhaps returning an error from appsec.Start (let's say appsec.NotStarted or something more specific which includes the reason) would be more clear.
Nothing needed in this PR, just some random feedback.
There was a problem hiding this comment.
Yes, I'm aware of this weirdness, due to how AppSec gets enabled for now: you can start/stop it, but it actually starts only if DD_APPSEC_ENABLED is true, hence the separate IsEnabled() call.
contrib/go-chi/chi.v5andcontrib/go-chi/chiintegrations.Note that I tried using the
http.TraceAndServe()function but it currently lacks the following features that I felt would be better in a separate PR: