-
Notifications
You must be signed in to change notification settings - Fork 293
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?
Changes from all commits
d1ab18d
bde374e
6f492b7
a1de372
0322905
3870050
408e6c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,27 @@ WHERE | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id = $1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FOR UPDATE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -- 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+51
to
+70
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| -- name: IntKeySetConfig :exec | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| INSERT INTO uik_config(id, config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VALUES ($1, $2) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |||||||||||||||||||||||||
| "io" | ||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||
| "unicode/utf8" | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| "github.com/expr-lang/expr/vm" | ||||||||||||||||||||||||||
| "github.com/google/uuid" | ||||||||||||||||||||||||||
|
|
@@ -33,6 +34,13 @@ type TxAble interface { | |||||||||||||||||||||||||
| BeginTx(ctx context.Context, opts *sql.TxOptions) (*sql.Tx, error) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||
| LogStatusSuccess = "success" | ||||||||||||||||||||||||||
| LogStatusParseError = "parse_error" | ||||||||||||||||||||||||||
| LogStatusExecError = "exec_error" | ||||||||||||||||||||||||||
| LogStatusSendError = "send_error" | ||||||||||||||||||||||||||
|
Comment on lines
+38
to
+41
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func NewHandler(db TxAble, intStore *integrationkey.Store, aStore *alert.Store, evt *event.Bus) *Handler { | ||||||||||||||||||||||||||
| return &Handler{intStore: intStore, db: db, alertStore: aStore, evt: evt} | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
@@ -88,6 +96,48 @@ func (h *Handler) handleAction(ctx context.Context, act gadb.UIKActionV1) (inser | |||||||||||||||||||||||||
| return didInsertSignals, nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // upsertUikLog will save a log of the most recent request to a universal integration key in the db | ||||||||||||||||||||||||||
| func (h *Handler) upsertUikLog( | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: https://go.dev/wiki/CodeReviewComments#initialisms
Suggested change
|
||||||||||||||||||||||||||
| ctx context.Context, | ||||||||||||||||||||||||||
| keyID uuid.UUID, | ||||||||||||||||||||||||||
| req *http.Request, | ||||||||||||||||||||||||||
| rawBody []byte, | ||||||||||||||||||||||||||
| status string, | ||||||||||||||||||||||||||
| errMsg string, | ||||||||||||||||||||||||||
|
Comment on lines
+101
to
+106
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Alternatively, we could just take h.upsertUIKLog(ctx, id, req, body, wrapError(err, ParseError))and then inspect it within the method |
||||||||||||||||||||||||||
| ) error { | ||||||||||||||||||||||||||
| if !expflag.ContextHas(ctx, expflag.UikLogs) { | ||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| const maxSize = 2048 | ||||||||||||||||||||||||||
| if len(rawBody) > maxSize { | ||||||||||||||||||||||||||
| rawBody = rawBody[:maxSize] | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| err := gadb.New(h.db).IntKeyLog(ctx, gadb.IntKeyLogParams{ | ||||||||||||||||||||||||||
| IntegrationKeyID: keyID, | ||||||||||||||||||||||||||
| Status: status, | ||||||||||||||||||||||||||
| RawBody: rawBody, | ||||||||||||||||||||||||||
| ContentType: sql.NullString{ | ||||||||||||||||||||||||||
| String: req.Header.Get("Content-Type"), | ||||||||||||||||||||||||||
| Valid: req.Header.Get("Content-Type") != "", | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| UserAgent: sql.NullString{ | ||||||||||||||||||||||||||
| String: req.UserAgent(), | ||||||||||||||||||||||||||
| Valid: req.UserAgent() != "", | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
Comment on lines
+120
to
+127
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||
| ErrorMessage: sql.NullString{ | ||||||||||||||||||||||||||
| String: errMsg, | ||||||||||||||||||||||||||
| Valid: errMsg != "", | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||
| fmt.Println("failed to log UIK request: ", err) | ||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // EventNewSignals is an event that is triggered when new signals are generated for a service. | ||||||||||||||||||||||||||
| type EventNewSignals struct { | ||||||||||||||||||||||||||
| ServiceID uuid.UUID | ||||||||||||||||||||||||||
|
|
@@ -117,11 +167,20 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| data, err := io.ReadAll(req.Body) | ||||||||||||||||||||||||||
| if errutil.HTTPError(ctx, w, err) { | ||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, nil, LogStatusParseError, "read error: "+err.Error()) | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use the logger rather than |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if !utf8.Valid(data) { | ||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, nil, LogStatusParseError, "invalid UTF-8") | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should log these, maybe remove the |
||||||||||||||||||||||||||
| errutil.HTTPError(ctx, w, validation.NewGenericError("invalid UTF-8 in request body")) | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var body any | ||||||||||||||||||||||||||
| err = json.Unmarshal(data, &body) | ||||||||||||||||||||||||||
| if errutil.HTTPError(ctx, w, validation.WrapError(err)) { | ||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, data, LogStatusParseError, "invalid json: "+err.Error()) | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -156,13 +215,16 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||||||||||||||||||||||||
| var vm vm.VM | ||||||||||||||||||||||||||
| actions, err := compiled.Run(&vm, env) | ||||||||||||||||||||||||||
| if errutil.HTTPError(ctx, w, validation.WrapError(err)) { | ||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, data, LogStatusExecError, "expression execution failed: "+err.Error()) | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| var insertedAny bool | ||||||||||||||||||||||||||
| for _, act := range actions { | ||||||||||||||||||||||||||
| inserted, err := h.handleAction(ctx, act) | ||||||||||||||||||||||||||
| if errutil.HTTPError(ctx, w, err) { | ||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, data, LogStatusSendError, "action failed: "+err.Error()) | ||||||||||||||||||||||||||
| errutil.HTTPError(ctx, w, err) | ||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| insertedAny = insertedAny || inserted | ||||||||||||||||||||||||||
|
|
@@ -172,5 +234,6 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||||||||||||||||||||||||
| event.Send(ctx, h.evt, EventNewSignals{ServiceID: permission.ServiceNullUUID(ctx).UUID}) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| _ = h.upsertUikLog(ctx, keyID, req, data, LogStatusSuccess, "") | ||||||||||||||||||||||||||
| w.WriteHeader(http.StatusNoContent) | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| -- +migrate Up | ||
|
|
||
| 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, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, |
||
| received_at timestamptz NOT NULL DEFAULT now(), | ||
|
|
||
| status text NOT NULL CHECK (status IN ( | ||
| 'success', | ||
| 'parse_error', | ||
| 'exec_error', | ||
| 'send_error' | ||
| )), | ||
|
|
||
| raw_body bytea, | ||
| content_type text, | ||
| user_agent text, | ||
| error_message text | ||
| ); | ||
|
|
||
| CREATE UNIQUE INDEX uik_logs_unique_key_id ON uik_logs (integration_key_id); | ||
|
|
||
| -- +migrate Down | ||
| DROP TABLE IF EXISTS uik_logs; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| // Code generated by expflag/cmd/tsgen DO NOT EDIT. | ||
|
|
||
| type ExpFlag = 'example' | 'univ-keys' | ||
| type ExpFlag = 'example' | 'uik-logs' | 'univ-keys' |
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
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_requestsfor the table, and the expflag I'm less sure, maybeuik-debug?