Add webhook error logging in Dashboard #1545
Conversation
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
Signed-off-by: downfa11 <[email protected]>
WalkthroughAdds webhook logging end-to-end: new WebhookLog types and converters, protobuf + OpenAPI schemas and ListWebhookLogs RPC, DB persistence (Mongo + in-memory) with indexes, webhook client change to return raw response body, wiring to persist failed/non-OK webhook attempts, and tests/bench updates. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminClient as Admin Client
participant AdminServer as AdminServer
participant DB as Database
participant Conv as Converter
AdminClient->>AdminServer: ListWebhookLogs(req{projectName,pageSize,pageToken,webhookType})
AdminServer->>AdminServer: resolve projectID from caller
AdminServer->>DB: ListWebhookLogs(projectID, webhookType, pageSize, offset)
DB->>DB: query, filter, sort by CreatedAt desc, apply offset/pageSize
DB-->>AdminServer: []*WebhookLogInfo
AdminServer->>Conv: ToWebhookLogs(logs)
Conv-->>AdminServer: []*api.WebhookLog
AdminServer->>AdminServer: generate nextPageToken(last.ID)
AdminServer-->>AdminClient: ListWebhookLogsResponse{logs, nextPageToken}
sequenceDiagram
participant Caller as Auth/Webhook Caller
participant Client as pkg/webhook.Client
participant Remote as Remote Webhook Server
participant DB as Database
Caller->>Client: Send(request)
Client->>Remote: HTTP POST (body)
Remote-->>Client: HTTP response (status, body) or error
alt status == 200 && no error
Client-->>Caller: (resp, 200, responseBody, nil)
Caller->>Caller: normal processing
else non-200 or error
Client-->>Caller: (nil, status, responseBody, error)
Caller->>Caller: build WebhookLogInfo{projectID,url,reqBody,status,responseBody,errorMessage,createdAt}
Caller->>DB: CreateWebhookLog(logInfo)
DB-->>Caller: ack / error
Caller->>Caller: continue (DB write errors logged/swallowed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)api/yorkie/v1/v1connect/yorkie.connect.go (2)
🪛 Checkov (3.2.334)api/docs/yorkie/v1/resources.openapi.yaml[medium] 237-245: Ensure that arrays have a maximum number of items (CKV_OPENAPI_21) api/docs/yorkie/v1/admin.openapi.yaml[medium] 1018-1026: Ensure that arrays have a maximum number of items (CKV_OPENAPI_21) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (8)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/webhook/client.go (1)
105-123: Critical: Response body read twice, breaking JSON unmarshaling.The response body is read on line 105, exhausting the reader. The subsequent
io.ReadAll(resp.Body)on line 118 will return an empty slice because HTTP response bodies can only be read once. This causes JSON unmarshaling on line 123 to operate on an empty body instead of the actual response, breaking all non-int response types.Apply this diff to use the already-read
responseBody:responseBody, err = io.ReadAll(resp.Body) if err != nil { return resp.StatusCode, fmt.Errorf("read response body: %w", err) } if !isExpectedStatus(resp.StatusCode) { return resp.StatusCode, ErrUnexpectedStatusCode } if _, ok := any(res).(int); ok { return resp.StatusCode, nil } - body, err := io.ReadAll(resp.Body) - if err != nil { - return resp.StatusCode, fmt.Errorf("send webhook: %w", err) - } - - if err := json.Unmarshal(body, &res); err != nil { + if err := json.Unmarshal(responseBody, &res); err != nil { logger := logging.From(ctx) if logger != nil { - logger.Errorf("send webhook url=%s, status=%d, body=%s", url, resp.StatusCode, string(body)) + logger.Errorf("send webhook url=%s, status=%d, body=%s", url, resp.StatusCode, string(responseBody)) } return resp.StatusCode, ErrInvalidJSONResponse }pkg/webhook/client_test.go (1)
292-292: Critical: Missing return value in function call.The call to
unreachableClient.Sendonly destructures 3 return values, but the function now returns 4 values (includingresponseBody). This will cause a compilation error.Apply this diff to fix the call:
- resp, statusCode, err := unreachableClient.Send(context.Background(), invalidJSONServer.URL, "", body, options) + resp, statusCode, _, err := unreachableClient.Send(context.Background(), invalidJSONServer.URL, "", body, options)
🧹 Nitpick comments (10)
server/backend/database/memory/indexes.go (2)
313-335: Add compound index to support webhook_type filtering.ListWebhookLogs filters by project and webhook_type; add an index on (ProjectID, WebhookType, CreatedAt) to avoid scans in memory backend.
tblWebhookLogs: { Name: tblWebhookLogs, Indexes: map[string]*memdb.IndexSchema{ "id": { Name: "id", Unique: true, Indexer: &memdb.StringFieldIndex{Field: "ID"}, }, "project_id": { Name: "project_id", Indexer: &memdb.StringFieldIndex{Field: "ProjectID"}, }, "project_id_created_at": { Name: "project_id_created_at", Indexer: &memdb.CompoundIndex{ Indexes: []memdb.Indexer{ &memdb.StringFieldIndex{Field: "ProjectID"}, &memdb.TimeFieldIndex{Field: "CreatedAt"}, }, }, }, + "project_id_type_created_at": { + Name: "project_id_type_created_at", + Indexer: &memdb.CompoundIndex{ + Indexes: []memdb.Indexer{ + &memdb.StringFieldIndex{Field: "ProjectID"}, + &memdb.StringFieldIndex{Field: "WebhookType"}, + &memdb.TimeFieldIndex{Field: "CreatedAt"}, + }, + }, + }, }, },
31-31: Naming consistency nit."webhookLogs" breaks the lowercase naming pattern used elsewhere (e.g., clusternodes, documents). Consider "webhooklogs" for consistency.
server/rpc/server_test.go (1)
259-261: Add tests for pagination and type filter.Cover non-empty results, NextPageToken propagation, PageToken consumption, and webhook_type filtering to prevent regressions.
server/backend/backend.go (1)
153-154: Fix variable name typo.Use eventWebhookManager consistently.
- eventWebhookManger := webhook.NewManager(pkgwebhook.NewClient[types.EventWebhookRequest, int](), db) + eventWebhookManager := webhook.NewManager(pkgwebhook.NewClient[types.EventWebhookRequest, int](), db) ... - EventWebhookManager: eventWebhookManger, + EventWebhookManager: eventWebhookManager,Also applies to: 202-204
server/backend/database/mongo/indexes.go (1)
206-219: Consider adding webhook_type to the compound index.Since
ListWebhookLogsfilters bywebhook_type(optional), including it in the compound index could improve query performance when filtering by type. The current index only coversproject_idandcreated_at.Consider an index like:
}, { name: ColWebhookLogs, indexes: []mongo.IndexModel{{ Keys: bson.D{ {Key: "project_id", Value: int32(1)}, + {Key: "webhook_type", Value: int32(1)}, {Key: "created_at", Value: int32(-1)}, // desc }, }, {This would efficiently support queries filtering by both project_id and webhook_type, while still allowing queries that filter by project_id alone (index prefix property).
server/rpc/testcases/testcases.go (1)
1606-1655: Consider testing with actual webhook logs.The current test only verifies the empty logs case and error handling. Consider adding a test case that creates actual webhook logs via the database and then verifies they are returned by ListWebhookLogs, similar to how TestWebhookLogs in client_test.go creates and retrieves logs.
This would provide more comprehensive coverage of the webhook log retrieval flow through the admin RPC layer.
server/backend/database/memory/database.go (1)
2272-2307: Use WebhookLogInfo.DeepCopy() for consistency.Lines 2289-2299 manually create a copy of the webhook log, but the WebhookLogInfo type already has a DeepCopy() method. Using it would be more consistent with the rest of the codebase (e.g., line 2343 in ListWebhookLogs uses DeepCopy).
- logCopy := &types.WebhookLogInfo{ - ID: webhookLog.ID, - ProjectID: webhookLog.ProjectID, - WebhookType: webhookLog.WebhookType, - WebhookURL: webhookLog.WebhookURL, - RequestBody: webhookLog.RequestBody, - StatusCode: webhookLog.StatusCode, - ResponseBody: responseBody, - ErrorMessage: webhookLog.ErrorMessage, - CreatedAt: webhookLog.CreatedAt, - } + logCopy := webhookLog.DeepCopy() + logCopy.ResponseBody = responseBodyNote: You'll need to truncate after the DeepCopy to ensure the stored copy has the truncated response body.
server/backend/database/mongo/client.go (1)
2387-2432: ListWebhookLogs: map ObjectID to hex; guard page size/offset
- Map
_idObjectID to hex string.- Clamp
pageSizeand sanitizeoffsetto avoid negative/oversized queries.Apply:
func (c *Client) ListWebhookLogs( @@ ) ([]*types.WebhookLogInfo, error) { filter := bson.M{"project_id": projectID} if webhookType != "" { filter["webhook_type"] = webhookType } + // Safety bounds + if pageSize <= 0 { + pageSize = 50 + } else if pageSize > 100 { + pageSize = 100 + } + if offset < 0 { + offset = 0 + } + opts := options.Find(). SetSort(bson.D{{Key: "created_at", Value: -1}}). SetLimit(int64(pageSize)). SetSkip(int64(offset)) @@ infos := make([]*types.WebhookLogInfo, len(docs)) for i, doc := range docs { infos[i] = &types.WebhookLogInfo{ - ID: types.ID(doc.ID), + ID: types.ID(doc.ID.Hex()), ProjectID: doc.ProjectID, WebhookType: doc.WebhookType, WebhookURL: doc.WebhookURL, RequestBody: doc.RequestBody, StatusCode: doc.StatusCode, ResponseBody: doc.ResponseBody, ErrorMessage: doc.ErrorMessage, CreatedAt: doc.CreatedAt, } } return infos, nil }Optional: consider redacting secrets in
WebhookURL(strip query/basic auth) before persisting to avoid leaking tokens.api/docs/yorkie/v1/admin.openapi.yaml (1)
1741-1766: Constrain pagination fieldsAdd reasonable bounds to guide clients and protect the backend.
Apply:
yorkie.v1.ListWebhookLogsRequest: @@ pageSize: additionalProperties: false description: "" title: page_size type: integer + minimum: 1 + maximum: 100 @@ webhookType: additionalProperties: false description: '"auth", "event", or "" for all' title: webhook_type type: string + enum: [auth, event]server/backend/webhook/manager.go (1)
106-126: Log non-200 and transport errors; use UTC timestampBehavior is correct. Prefer UTC for consistent storage.
Apply:
- CreatedAt: time.Now(), + CreatedAt: time.Now().UTC(),Optional: define a constant for
"event"to avoid typos, or reuse an existing enumerated type if present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/yorkie/v1/admin.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (19)
api/converter/to_pb.go(1 hunks)api/docs/yorkie/v1/admin.openapi.yaml(5 hunks)api/types/webhook_log.go(1 hunks)api/yorkie/v1/admin.proto(3 hunks)api/yorkie/v1/v1connect/admin.connect.go(9 hunks)pkg/webhook/client.go(3 hunks)pkg/webhook/client_test.go(9 hunks)server/backend/backend.go(1 hunks)server/backend/database/database.go(1 hunks)server/backend/database/memory/database.go(1 hunks)server/backend/database/memory/indexes.go(2 hunks)server/backend/database/mongo/client.go(2 hunks)server/backend/database/mongo/client_test.go(2 hunks)server/backend/database/mongo/indexes.go(3 hunks)server/backend/webhook/manager.go(5 hunks)server/rpc/admin_server.go(1 hunks)server/rpc/auth/webhook.go(2 hunks)server/rpc/server_test.go(1 hunks)server/rpc/testcases/testcases.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
api/converter/to_pb.go (3)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)api/yorkie/v1/admin.pb.go (3)
WebhookLog(2771-2784)WebhookLog(2799-2799)WebhookLog(2814-2816)api/types/id.go (1)
ID(37-37)
server/backend/backend.go (2)
server/backend/webhook/manager.go (1)
NewManager(49-55)pkg/webhook/client.go (1)
NewClient(65-69)
server/backend/database/database.go (2)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)api/types/id.go (1)
ID(37-37)
server/rpc/admin_server.go (5)
api/yorkie/v1/admin.pb.go (6)
ListWebhookLogsRequest(2645-2654)ListWebhookLogsRequest(2669-2669)ListWebhookLogsRequest(2684-2686)ListWebhookLogsResponse(2716-2723)ListWebhookLogsResponse(2738-2738)ListWebhookLogsResponse(2753-2755)server/projects/projects.go (1)
GetProject(66-78)api/types/id.go (1)
ID(37-37)api/converter/to_pb.go (1)
ToWebhookLogs(739-745)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)
server/backend/database/mongo/client_test.go (1)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)
server/backend/database/memory/database.go (2)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)api/types/id.go (1)
ID(37-37)
server/rpc/server_test.go (1)
server/rpc/testcases/testcases.go (1)
RunAdminListWebhookLogsTest(1607-1655)
server/rpc/auth/webhook.go (3)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)api/types/id.go (1)
ID(37-37)server/logging/context.go (1)
From(32-43)
server/backend/database/mongo/client.go (3)
api/types/id.go (1)
ID(37-37)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)server/backend/database/mongo/indexes.go (1)
ColWebhookLogs(48-48)
server/rpc/testcases/testcases.go (7)
api/yorkie/v1/v1connect/admin.connect.go (1)
AdminServiceClient(127-154)admin/auth.go (1)
AuthInterceptor(31-33)test/helper/helper.go (4)
TestSlugName(410-439)AdminUser(74-74)AdminPassword(75-75)CreateProject(622-628)api/yorkie/v1/admin.pb.go (9)
LogInRequest(196-203)LogInRequest(218-218)LogInRequest(233-235)CreateProjectRequest(492-498)CreateProjectRequest(513-513)CreateProjectRequest(528-530)ListWebhookLogsRequest(2645-2654)ListWebhookLogsRequest(2669-2669)ListWebhookLogsRequest(2684-2686)server/rpc/connecthelper/errors.go (1)
CodeOf(59-76)server/backend/database/database.go (1)
ErrProjectNotFound(40-40)api/converter/errors.go (1)
ErrorCodeOf(11-27)
api/yorkie/v1/v1connect/admin.connect.go (1)
api/yorkie/v1/admin.pb.go (6)
ListWebhookLogsRequest(2645-2654)ListWebhookLogsRequest(2669-2669)ListWebhookLogsRequest(2684-2686)ListWebhookLogsResponse(2716-2723)ListWebhookLogsResponse(2738-2738)ListWebhookLogsResponse(2753-2755)
server/backend/webhook/manager.go (4)
server/backend/database/database.go (1)
Database(74-439)api/types/event_webhook.go (3)
EventWebhookRequest(50-53)EventWebhookType(31-31)NewRequestBody(56-70)api/types/id.go (1)
ID(37-37)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)
🔇 Additional comments (19)
server/rpc/auth/webhook.go (1)
71-91: Redact RequestBody token and sanitize webhook URL before logging; also use UTC timestamp.RequestBody currently contains the auth token directly (it's marshaled from
reqcontaining the Token field). Additionally, webhook URLs aren't sanitized and could contain credentials. The 10KB ResponseBody truncation is already handled at the database layer, so no duplication needed.Apply this simpler patch to both webhook.go (lines 71-91) and manager.go (lines 110-126):
if err != nil || status != http.StatusOK { errorMessage := "" if err != nil { errorMessage = err.Error() } + // Sanitize URL by removing credentials/query + safeURL := prj.AuthWebhookURL + if u, perr := url.Parse(prj.AuthWebhookURL); perr == nil { + u.User = nil + u.RawQuery = "" + safeURL = u.String() + } webhookLog := &types.WebhookLogInfo{ ProjectID: prj.ID, WebhookType: "auth", - WebhookURL: prj.AuthWebhookURL, - RequestBody: body, + WebhookURL: safeURL, + RequestBody: body, // Token already in marshaled form; database layer truncates ResponseBody - CreatedAt: time.Now(), + CreatedAt: time.Now().UTC(), } if logErr := be.DB.CreateWebhookLog(ctx, webhookLog); logErr != nil {For event webhooks in manager.go, use
attr.URLinstead ofprj.AuthWebhookURL. Add"net/url"import if not present.For deeper redaction of the Token field itself, consider a pre-marshal approach or database-layer filtering.
api/converter/to_pb.go (1)
724-745: LGTM!The webhook log converters follow the established patterns in this file and correctly map internal types to protobuf representations.
server/backend/database/mongo/client_test.go (1)
239-283: LGTM!The webhook logs tests provide good coverage for both creation and retrieval scenarios, including filtering by webhook type. The tests follow the established patterns in this file.
api/types/webhook_log.go (1)
5-40: LGTM!The WebhookLogInfo type and DeepCopy implementation are well-designed. The DeepCopy correctly handles nil checks and creates deep copies of byte slices, ensuring safe concurrent access.
api/yorkie/v1/admin.proto (2)
62-62: LGTM!The new ListWebhookLogs RPC follows the established patterns in the AdminService.
295-316: LGTM!The webhook log messages are well-designed with proper pagination support. The optional webhook_type filter allows querying all logs or filtering by specific type.
server/backend/database/memory/database.go (1)
2309-2351: LGTM!The ListWebhookLogs implementation correctly handles filtering, pagination, and sorting. The use of DeepCopy ensures safe concurrent access.
server/backend/database/mongo/client.go (1)
2357-2385: CreateWebhookLog: aligns with 10KB truncation; set ID from ObjectIDTruncation logic looks good. With the
_idfix, theInsertedIDcast remains valid. No further changes needed here.api/docs/yorkie/v1/admin.openapi.yaml (3)
218-229: Add ListWebhookLogs path: OKPath wiring and tags look consistent with existing AdminService routes.
481-489: Request body schema reference: OKRequestBody component correctly references the new request schema.
725-733: Response schema reference: OKResponse component correctly references the new response schema.
api/yorkie/v1/v1connect/admin.connect.go (6)
121-124: Procedure constant added: OKConstant name and value align with service/method.
153-154: Client interface method: OKNew method signature matches generated request/response types.
291-296: Client initialization: OKClient constructed with correct procedure path.
454-457: Client method implementation: OKDelegates to underlying client; consistent with other methods.
620-624: Handler wiring and routing: OKUnary handler and switch case registered correctly.
788-790: Unimplemented stub: OKReturns CodeUnimplemented as expected.
server/backend/webhook/manager.go (2)
45-55: Inject DB into Manager: OKConstructor and struct updated coherently; no issues.
74-77: Immediate call path: OKLimiter bypass path invokes SendWebhook directly; no double-send.
Signed-off-by: downfa11 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
server/backend/database/mongo/indexes.go (2)
47-49: Collection naming consistencyAll other collections are lowercase; "webhookLogs" is camelCase. Prefer lowercase to match convention and avoid surprises in tooling and queries.
Apply:
- ColWebhookLogs = "webhookLogs" + ColWebhookLogs = "webhooklogs"
206-220: Add index for queries without webhook_type; keep TTL as-isListWebhookLogs often filters only by projectID and sorts by created_at desc. The current compound index won’t support that sort when webhook_type is omitted. Add (project_id, created_at:-1).
Apply:
{ name: ColWebhookLogs, indexes: []mongo.IndexModel{{ Keys: bson.D{ {Key: "project_id", Value: int32(1)}, {Key: "webhook_type", Value: int32(1)}, {Key: "created_at", Value: int32(-1)}, // desc }, }, { + // Support queries that filter only by project_id and sort by created_at desc. + Keys: bson.D{ + {Key: "project_id", Value: int32(1)}, + {Key: "created_at", Value: int32(-1)}, // desc + }, + }, { Keys: bson.D{ {Key: "created_at", Value: int32(1)}, }, Options: options.Index().SetExpireAfterSeconds(2592000), // TTL: 30day }}, },pkg/webhook/client_test.go (1)
292-297: Optional: also assert responseBody for invalid JSONAssert that responseBody is non‑empty to guarantee we capture bodies that fail JSON decoding (useful for DB log tests).
server/backend/database/mongo/client.go (2)
2357-2385: Small hardening: default CreatedAt and named limitGuard zero timestamps and make the 10KB cap a named const for reuse.
Apply:
+const maxRespBodyBytes = 10 * 1024 + func (c *Client) CreateWebhookLog( ctx context.Context, webhookLog *types.WebhookLogInfo, ) error { responseBody := webhookLog.ResponseBody - if len(responseBody) > 10*1024 { - responseBody = responseBody[:10*1024] // 10kb + if len(responseBody) > maxRespBodyBytes { + responseBody = responseBody[:maxRespBodyBytes] } + if webhookLog.CreatedAt.IsZero() { + webhookLog.CreatedAt = gotime.Now().UTC() + }
2387-2441: Offset pagination may not scale; prefer seek paginationSkip/limit with large offsets degrades on big collections. Consider seek pagination using (created_at, _id) tuple and a “cursor/after” token. Align indexes accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pkg/webhook/client.go(4 hunks)pkg/webhook/client_test.go(10 hunks)server/backend/backend.go(2 hunks)server/backend/database/database.go(1 hunks)server/backend/database/memory/indexes.go(2 hunks)server/backend/database/mongo/client.go(2 hunks)server/backend/database/mongo/indexes.go(3 hunks)server/backend/webhook/manager.go(5 hunks)server/rpc/admin_server.go(2 hunks)test/bench/webhook_bench_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/rpc/admin_server.go
- server/backend/backend.go
- server/backend/database/database.go
- server/backend/database/memory/indexes.go
- pkg/webhook/client.go
🧰 Additional context used
🧬 Code graph analysis (3)
test/bench/webhook_bench_test.go (1)
server/backend/webhook/manager.go (1)
NewManager(49-55)
server/backend/database/mongo/client.go (3)
api/types/id.go (1)
ID(37-37)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)server/backend/database/mongo/indexes.go (1)
ColWebhookLogs(48-48)
server/backend/webhook/manager.go (3)
api/types/event_webhook.go (3)
EventWebhookRequest(50-53)EventWebhookType(31-31)NewRequestBody(56-70)api/types/id.go (1)
ID(37-37)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: bench
- GitHub Check: load-test
🔇 Additional comments (4)
pkg/webhook/client_test.go (1)
105-105: LGTM on Send signature updatesTests correctly adapt to (resp, statusCode, responseBody, err). Assertions look sound and improve observability.
Also applies to: 117-117, 129-129, 141-141, 168-168, 185-185, 210-210, 227-227, 254-259, 266-271, 292-292
server/backend/database/mongo/client.go (1)
62-72: Storage model looks goodUsing ObjectID with
omitemptyavoids duplicate key issues and lets Mongo assign IDs.server/backend/webhook/manager.go (1)
49-55: Event manager changes look goodConstructor wiring, limiter integration, and error/status handling are correct. Logging only on failure/non‑OK is appropriate.
If you plan to log auth‑webhook failures too, ensure the analogous path sets
WebhookType: "auth"consistently.Also applies to: 59-87, 136-143
test/bench/webhook_bench_test.go (1)
1-1: Go version requirement confirmedThe go.mod declares
go 1.23.0, which satisfies the Go 1.22+ requirement for range-over-integers syntax. No changes needed.
Signed-off-by: downfa11 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/backend/database/mongo/indexes.go (1)
206-220: Indexes are well-designed for the use case.The compound index efficiently supports queries by project ID and webhook type with reverse chronological ordering (most recent first). The TTL index is correctly configured as a single-field ascending index to automatically expire logs after 30 days.
Minor: Consider adding shard key comment for consistency.
For consistency with other collections in this file (lines 121, 127, 133, 150, etc.), consider annotating
project_idas the shard key:indexes: []mongo.IndexModel{{ Keys: bson.D{ - {Key: "project_id", Value: int32(1)}, + {Key: "project_id", Value: int32(1)}, // shard key {Key: "webhook_type", Value: int32(1)}, {Key: "created_at", Value: int32(-1)}, // desc },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/backend/database/memory/database.go(1 hunks)server/backend/database/mongo/client.go(2 hunks)server/backend/database/mongo/indexes.go(3 hunks)server/backend/webhook/manager.go(5 hunks)test/bench/webhook_bench_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/backend/database/mongo/client.go
🧰 Additional context used
🧬 Code graph analysis (3)
test/bench/webhook_bench_test.go (3)
server/backend/database/memory/database.go (1)
New(44-54)server/backend/backend.go (1)
New(87-211)server/backend/webhook/manager.go (1)
NewManager(49-55)
server/backend/webhook/manager.go (5)
server/backend/database/database.go (1)
Database(74-439)pkg/webhook/client.go (1)
Client(59-61)api/types/event_webhook.go (3)
EventWebhookRequest(50-53)EventWebhookType(31-31)NewRequestBody(56-70)api/types/id.go (1)
ID(37-37)api/types/webhook_log.go (1)
WebhookLogInfo(5-15)
server/backend/database/memory/database.go (2)
api/types/webhook_log.go (1)
WebhookLogInfo(5-15)api/types/id.go (1)
ID(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: bench
- GitHub Check: complex-test
- GitHub Check: build
- GitHub Check: load-test
🔇 Additional comments (11)
server/backend/database/mongo/indexes.go (2)
47-48: LGTM!The constant definition and documentation follow the established pattern and naming conventions used throughout the file.
62-62: LGTM!The addition to the Collections slice is correct.
test/bench/webhook_bench_test.go (3)
33-33: LGTM! Import correctly added for in-memory database support.The import enables the benchmark to create in-memory database instances, addressing the nil database issue flagged in previous reviews.
106-110: LGTM! In-memory database properly initialized and passed.The benchmark now creates and passes a non-nil database instance to
SendWebhook, resolving the panic risk flagged in the previous review.Also applies to: 132-133
154-159: LGTM! Consistent in-memory database setup for Manager benchmark.The setup mirrors the pattern in
benchmarkSendWebhookand properly provides a database instance toNewManager, preventing the nil-db panic.Also applies to: 172-172
server/backend/database/memory/database.go (2)
2272-2298: LGTM! CreateWebhookLog correctly handles ID generation, truncation, and storage.The method properly:
- Generates IDs when missing
- Truncates response bodies to 10KB to limit storage
- Creates deep copies to prevent mutation
- Handles transactions with proper error wrapping
2300-2342: LGTM! ListWebhookLogs correctly implements filtering, pagination, and sorting.The method properly:
- Filters by project and webhook type
- Applies offset and page size pagination
- Returns deep copies for thread safety
- Sorts by creation time (newest first)
The in-memory sort is acceptable for this implementation's use case (tests and temporary storage).
server/backend/webhook/manager.go (4)
29-29: LGTM! Database field correctly added to Manager.The
dbfield is properly declared, accepted inNewManager, and initialized, enabling webhook log persistence.Also applies to: 45-45, 49-49, 53-53
66-67: LGTM! Send method correctly propagates database and project ID.Both the immediate and debounced execution paths properly pass
projectIDanddbtoSendWebhook, maintaining consistency.Also applies to: 76-84
105-106: LGTM! SendWebhook signature extended to accept database and project ID.The signature changes enable webhook log persistence for debugging webhook integration failures.
114-135: LGTM! Webhook logging correctly guards against nil database and handles errors.The implementation properly:
- Guards against nil
dbbefore logging (line 115) — resolving the panic risk from the previous review- Creates comprehensive webhook log entries on failures
- Logs any persistence errors without failing the webhook send
- Preserves the original error flow
Signed-off-by: downfa11 <[email protected]>
hackerwins
force-pushed
the
main
branch
2 times, most recently
from
6f9ef37 to
df0b450
Compare
1 participant
What this PR does / why we need it:
This PR adds webhook error logging functionality to help users debug webhook integration issues in PaaS environments.
When webhook servers return errors, the system now:
This enables dashboard users to troubleshoot webhook failures without requiring direct server access.
Which issue(s) this PR fixes:
Fixes #1540
Special notes for your reviewer:
api/yorkie/v1/admin.protoDoes this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores