-
Notifications
You must be signed in to change notification settings - Fork 291
uik: save most recent request to universal integration key #4395
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| if !utf8.Valid(data) { | ||
| _ = h.upsertUikLog(ctx, keyID, req, nil, LogStatusParseError, "invalid UTF-8") |
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.
we should log these, maybe remove the error return value from the method and handle the error internally (by logging it)
|
|
||
| CREATE TABLE uik_logs ( | ||
| id uuid PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| integration_key_id uuid NOT NULL REFERENCES integration_keys(id) ON DELETE CASCADE, |
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.
nit/preference: we can just add UNIQUE to the column definition at table creation time instead of creating an explicit index below
integration_key_id uuid NOT NULL UNIQUE REFERENCES integration_keys(id) ON DELETE CASCADE,| ctx context.Context, | ||
| keyID uuid.UUID, | ||
| req *http.Request, | ||
| rawBody []byte, | ||
| status string, | ||
| errMsg string, |
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.
nit: with so many args it may be worth using a struct, but if we use explicit types it could at least prevent mixing up status/err and enforce explicit values
| ctx context.Context, | |
| keyID uuid.UUID, | |
| req *http.Request, | |
| rawBody []byte, | |
| status string, | |
| errMsg string, | |
| ctx context.Context, | |
| keyID uuid.UUID, | |
| req *http.Request, | |
| rawBody []byte, | |
| status RequestStatus, | |
| err error, |
Alternatively, we could just take err and wrap it appropriately to tell the difference
h.upsertUIKLog(ctx, id, req, body, wrapError(err, ParseError))and then inspect it within the method
| } | ||
|
|
||
| // upsertUikLog will save a log of the most recent request to a universal integration key in the db | ||
| func (h *Handler) upsertUikLog( |
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.
nit: https://go.dev/wiki/CodeReviewComments#initialisms
| func (h *Handler) upsertUikLog( | |
| func (h *Handler) upsertUIKLog( |
| LogStatusSuccess = "success" | ||
| LogStatusParseError = "parse_error" | ||
| LogStatusExecError = "exec_error" | ||
| LogStatusSendError = "send_error" |
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.
maybe these could be error types? since success is sort of on it's own
| ContentType: sql.NullString{ | ||
| String: req.Header.Get("Content-Type"), | ||
| Valid: req.Header.Get("Content-Type") != "", | ||
| }, | ||
| UserAgent: sql.NullString{ | ||
| String: req.UserAgent(), | ||
| Valid: req.UserAgent() != "", | ||
| }, |
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.
Should we just make these non-null and allow the empty string? not a huge deal but would simplify things
| data, err := io.ReadAll(req.Body) | ||
| if errutil.HTTPError(ctx, w, err) { | ||
| _ = h.upsertUikLog(ctx, keyID, req, nil, LogStatusParseError, "read error: "+err.Error()) | ||
| return |
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.
use the logger rather than fmt.Print* so it goes to the right place and format
| const ( | ||
| Example Flag = "example" | ||
| UnivKeys Flag = "univ-keys" | ||
| UikLogs Flag = "uik-logs" |
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.
nit: https://go.dev/wiki/CodeReviewComments#initialisms
| UikLogs Flag = "uik-logs" | |
| UIKLogs Flag = "uik-logs" |
also, I'm wondering if there's a different way to describe this than "logs" and "logs support" since it doesn't have anything to do with application logs, and the table itself only records the last status/result, and maintains no running "log" of events.
Maybe uik_requests for the table, and the expflag I'm less sure, maybe uik-debug?
| -- name: IntKeyLog :exec | ||
| INSERT INTO uik_logs ( | ||
| integration_key_id, | ||
| content_type, | ||
| user_agent, | ||
| raw_body, | ||
| status, | ||
| error_message, | ||
| received_at | ||
| ) VALUES ( | ||
| $1, $2, $3, $4, $5, $6, $7 | ||
| ) | ||
| ON CONFLICT (integration_key_id) DO UPDATE | ||
| SET | ||
| content_type = EXCLUDED.content_type, | ||
| user_agent = EXCLUDED.user_agent, | ||
| raw_body = EXCLUDED.raw_body, | ||
| status = EXCLUDED.status, | ||
| error_message = EXCLUDED.error_message, | ||
| received_at = EXCLUDED.received_at; |
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.
For testing/consistency, we'll want to use DB time -- if we pass in the received_at time, we'll need to do a separate query to get the current time. Instead, we can just rely on the default of now() and on the conflict update set the value to the current DB time.
| -- name: IntKeyLog :exec | |
| INSERT INTO uik_logs ( | |
| integration_key_id, | |
| content_type, | |
| user_agent, | |
| raw_body, | |
| status, | |
| error_message, | |
| received_at | |
| ) VALUES ( | |
| $1, $2, $3, $4, $5, $6, $7 | |
| ) | |
| ON CONFLICT (integration_key_id) DO UPDATE | |
| SET | |
| content_type = EXCLUDED.content_type, | |
| user_agent = EXCLUDED.user_agent, | |
| raw_body = EXCLUDED.raw_body, | |
| status = EXCLUDED.status, | |
| error_message = EXCLUDED.error_message, | |
| received_at = EXCLUDED.received_at; | |
| -- name: IntKeyLog :exec | |
| INSERT INTO uik_logs ( | |
| integration_key_id, | |
| content_type, | |
| user_agent, | |
| raw_body, | |
| status, | |
| error_message | |
| ) VALUES ( | |
| $1, $2, $3, $4, $5, $6 | |
| ) | |
| ON CONFLICT (integration_key_id) DO UPDATE | |
| SET | |
| content_type = EXCLUDED.content_type, | |
| user_agent = EXCLUDED.user_agent, | |
| raw_body = EXCLUDED.raw_body, | |
| status = EXCLUDED.status, | |
| error_message = EXCLUDED.error_message, | |
| received_at = now(); |
make checkto catch common errors. Fixed any that came up.Description:
Adds new
uik_logstable for storing the most recent requests to universal integration keys.Which issue(s) this PR fixes:
Part of #4187
Out of Scope:
Follow up work will be to make logs viewable in the UI to aid in debugging integrations with universal integration keys.
Additional Considerations:
This implementation focuses on capturing and storing the most recent request log for each universal integration key to support basic debugging functionality as a MVP.
In the future, we may want to expand this system to store multiple logs per key, with smarter deduplication strategies or time-based retention. Ideas include: