-
Notifications
You must be signed in to change notification settings - Fork 440
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
dyngo: dynamically register listeners only if they're needed #2394
Conversation
BenchmarksBenchmark execution time: 2024-02-05 14:00:11 Comparing candidate commit b0160e2 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 37 metrics, 2 unstable metrics. scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24
scenario:BenchmarkStartRequestSpan-24
|
da82cfe
to
3d6b5e6
Compare
With a somewhat significant refactor, and use of Go generics, it is possible to have AppSec listeners self-install if, and only if a contrib that may emit events they listen to is loaded. This avoid logging uncanny messages in debug mode, where we previously claimed to register all available listeners (GraphQL, gRPC, HTTP), despite some of them being nonsensical in the user's application context. It also allows the compiler to perform better dead code elimination, or even entirely skipping packages corresponding to the unneeded listeners.
6f9f8bb
to
9814dc4
Compare
…vation-week # Conflicts: # internal/appsec/waf.go
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.
Cool rework!
̶I̶'̶m̶ ̶t̶h̶i̶n̶k̶i̶n̶g̶ ̶w̶e̶ ̶c̶a̶n̶ ̶g̶e̶t̶ ̶r̶i̶d̶ ̶o̶f̶ ̶I̶s̶A̶r̶g̶O̶r̶f̶/̶I̶s̶R̶e̶s̶u̶l̶t̶O̶f̶ ̶i̶m̶p̶l̶e̶m̶s̶ ̶o̶v̶e̶r̶a̶l̶l̶ ̶a̶n̶d̶ ̶j̶u̶s̶t̶ ̶u̶s̶e̶ ̶t̶h̶e̶ ̶i̶n̶t̶e̶r̶f̶a̶c̶e̶s̶ ̶t̶o̶ ̶e̶n̶f̶o̶r̶c̶e̶ ̶t̶y̶p̶e̶ ̶c̶o̶n̶s̶t̶r̶a̶i̶n̶t̶,̶ ̶l̶e̶t̶ ̶m̶e̶ ̶k̶n̶o̶w̶ ̶w̶h̶a̶t̶ ̶y̶o̶u̶ ̶t̶h̶i̶n̶k̶.̶ ̶M̶a̶i̶n̶l̶y̶ ̶i̶t̶ ̶j̶u̶s̶t̶ ̶r̶e̶m̶o̶v̶e̶s̶ ̶t̶h̶e̶ ̶n̶e̶e̶d̶ ̶t̶o̶ ̶i̶m̶p̶l̶e̶m̶e̶n̶t̶ ̶t̶h̶o̶s̶e̶ ̶e̶m̶p̶t̶y̶ ̶f̶u̶n̶c̶t̶i̶o̶n̶s̶ ̶f̶o̶r̶ ̶e̶a̶c̶h̶ ̶a̶r̶g̶/̶r̶e̶s̶ ̶t̶y̶p̶e̶
Other than that LGTM, spotted a few exported funcs that could use a description for consistency.
EDIT: scratch that first part, making it work would do more harm than good
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.
Didn't review the appsec code, but I don't see any major changes in contrib
other than using a different package, so I'll trust that others have reviewed the rest.
Pull Request is not mergeable
Pull Request is not mergeable
4eb7fdd
to
b0160e2
Compare
Congrats - major API change embracing the Go generics the right way 👏 This was envisioned in the original PR design as a major improvement :-) |
/merge |
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
🚂 MergeQueue Added to the queue. This build is going to start soon! (estimated merge in less than 0s) Use |
🚂 MergeQueue This pull request was merged directly. |
What does this PR do?
With a somewhat significant refactor, and use of Go generics, it is possible to have AppSec listeners self-install if, and only if a contrib that may emit events they listen to is loaded.
Motivation
This avoid logging uncanny messages in debug mode, where we previously claimed to register all available listeners (GraphQL, gRPC, HTTP), despite some of them being nonsensical in the user's application context.
It also allows the compiler to perform better dead code elimination, or even entirely skipping packages corresponding to the unneeded listeners.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!