Custom event tracking - #286
Conversation
17f3482 to
23ffc5f
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
23ffc5f to
29ebef3
Compare
925d1bd to
03c8326
Compare
|
|
||
| @config.logger.debug("Reporting #{event_type.upcase} event") | ||
|
|
||
| req = Net::HTTP::Post.new("/api/runtime/events", default_headers) |
There was a problem hiding this comment.
These events can only be sent to zen.aikido.dev, the old runtime API does not support this.
There was a problem hiding this comment.
That's quite an important point for the other agents too. If the agent is unable to connect to zen.aikido.dev, we need to make it really clear that the user events aren't going to work. I wonder if we need a log here, @timokoessler?
There was a problem hiding this comment.
Indeed, makes sense. Maybe log if user calls the track() API for the first time? cc. @hansott.
| def send_user_event(event) | ||
| event_type = "user_event" | ||
|
|
||
| if @rate_limiter.throttle?(event_type) |
There was a problem hiding this comment.
As these events are not send to Aikido Core, maybe not re-use the same rate limiter? (I think this is the case)
03c8326 to
a13654a
Compare
8bf6fbd to
306b86f
Compare
c71014a to
e32b408
Compare
| response = http.request(request) | ||
|
|
||
| case response | ||
| when Net::HTTPNoContent |
There was a problem hiding this comment.
Empty when Net::HTTPNoContent branch is verbose; combine Net::HTTPNoContent with Net::HTTPSuccess in the same when clause or explicitly return nil to simplify the case handling.
Details
✨ AI Reasoning
In the request method an explicit when branch for Net::HTTPNoContent is added with an empty body. This is more verbose than necessary: treating Net::HTTPNoContent together with Net::HTTPSuccess (and handling the empty-body case by returning nil) is a simpler, equally readable approach. Combining them would reduce the number of case arms and clarify intent.
🔧 How do I fix it?
Rewrite the snippet in the simpler, behavior-equivalent form: return a boolean expression directly instead of if cond return true else return false, avoid using lists when they are guaranteed to contain one element, etc.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| raise | ||
| end | ||
|
|
||
| def send_user_event(event) |
There was a problem hiding this comment.
APIClient#send_user_event overlaps in name with APIClient#report but uses a different endpoint and behavior; clarify intent or rename to reflect the specialized behavior (e.g., post_user_event_to_realtime).
| def send_user_event(event) | |
| # Posts a user event to the realtime settings updates endpoint, using | |
| # separate rate limiting from standard event reporting. | |
| def send_user_event(event) |
Details
✨ AI Reasoning
APIClient#send_user_event was added to send user events but its behavior differs from APIClient#report (different rate-limiter key, different base_url). The method name closely resembles report, making the distinct responsibilities unclear without inspecting implementation.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| end | ||
| end | ||
|
|
||
| # @param event [Aikido::Zen::Tracked] |
There was a problem hiding this comment.
Agent#send_user_event purpose is unclear relative to Agent#report. Clarify how it differs (intent, lifecycle, and expected callers) and fix the incorrect/undefined doc type Aikido::Zen::Tracked.
Show fix
| # @param event [Aikido::Zen::Tracked] | |
| # Asynchronously sends user-triggered events to Aikido, skipping enqueue | |
| # if reporting is disabled. | |
| # | |
| # @param event [Object] a user event payload. |
Details
✨ AI Reasoning
A method was added that enqueues and sends user events from the Agent layer. Its name send_user_event overlaps with the existing report method, and the doc annotation references Aikido::Zen::Tracked (not defined here). It's not obvious how send_user_event differs from report or when to use one vs the other without reading its implementation.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| end | ||
| end | ||
|
|
||
| class UserEvent |
There was a problem hiding this comment.
Todo: This event needs to be updated to follow new spec: https://github.com/AikidoSec/zen-specs/blob/main/features/custom_event_tracking.md
This change adds support for tracking named user events, sent from application code using
Aikido::Zen.track_user_event. User events include the event name, user ID, and IP address.This change extends and should be reviewed after #285.
Summary by Aikido
🚀 New Features
⚡ Enhancements
More info