-
Notifications
You must be signed in to change notification settings - Fork 233
Add: Sandbox Logs V2 endpoint #1647
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: main
Are you sure you want to change the base?
Conversation
- Introduced new endpoint (GET /v2/sandboxes/{sandboxID}/logs) to fetch structured logs for a specific sandbox.
- Added corresponding middleware and request handling in the server interface.
- Defined new parameters and response types for the V2 sandbox logs.
- Updated OpenAPI specifications to include the new endpoint and response structure.
- Merged QuerySandboxLogs and QuerySandboxLogsV2 into a single method with an updated signature to include the direction parameter. - Removed the old QuerySandboxLogs method implementation to streamline the logging query process.
- Consolidated V2 sandbox logs handling into V1 API, removing the deprecated V2 endpoint. - Updated method signatures to include optional parameters for start, end, limit, and direction. - Adjusted response structures to streamline log entries and improve consistency. - Enhanced OpenAPI specifications to reflect the changes in parameters and responses.
| cursor := time.UnixMilli(*params.Cursor) | ||
| if direction == api.LogsDirectionForward { | ||
| start = cursor | ||
| end = cursor.Add(sandboxLogsMaxTimeRange) |
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.
When direction is forward and cursor is near the present, end can extend into the future (line 52). This could cause unexpected behavior. Consider capping end at time.Now() to prevent querying future timestamps.
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.
invalid, template build logs uses the same pattern, querying future timestamps in loki is harmless
|
|
||
| start, end := time.Now().Add(-sandboxLogsMaxTimeRange), time.Now() | ||
| if params.Cursor != nil { | ||
| cursor := time.UnixMilli(*params.Cursor) |
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.
The cursor timestamp is not validated before use. While the OpenAPI spec defines minimum: 0, consider adding validation to ensure the cursor is within a reasonable range (e.g., not older than 7 days, not in the future) to prevent excessive queries or invalid time ranges.
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.
i think this is fine, it's handled the same in template build logs, excessive cursors just return empty results
| } | ||
|
|
||
| if res.JSON200 == nil { | ||
| telemetry.ReportCriticalError(ctx, "error when returning logs for sandbox", fmt.Errorf("unexpected response for sandbox '%s': %s", sandboxID, string(res.Body))) |
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.
Converting the entire response body to string for error logging could expose sensitive data or cause memory issues with large responses. Consider logging just the HTTP status code and truncating the body, or using a structured error response if available.
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.
taken from sandbox logs v1
dobrac
left a comment
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.
lgtm, just few nits
| "500": | ||
| $ref: "#/components/responses/500" | ||
|
|
||
| /sandboxes/{sandboxID}/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.
can you please make this endpoint deprecated?
| properties: | ||
| logs: | ||
| default: [] | ||
| description: Sandbox 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.
why removal of this?
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.
slipped through, thanks
| ctx := c.Request.Context() | ||
| sandboxID = utils.ShortID(sandboxID) | ||
|
|
||
| team := c.Value(auth.TeamContextKey).(*types.Team) |
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.
you can use GetTeamInfo instead
| team := c.Value(auth.TeamContextKey).(*types.Team) | ||
|
|
||
| telemetry.SetAttributes(ctx, | ||
| attribute.String("instance.id", sandboxID), |
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 telemetry.WithSandboxID instead
| d := apiedge.V1SandboxLogsParamsDirectionForward | ||
| edgeDirection = &d |
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.
you can use utils.ToPtr instead
|
Main PR with API migration is merged, I would wait for this #1713 one, and then I will apply your changes to the current main. |
Note
Adds a new logs API focused on structured entries and navigable ranges.
GET /v2/sandboxes/{sandboxID}/logswithcursor,limit, anddirection(forward/backward); returnsSandboxLogsV2Response(structuredlogsonly)GET /sandboxes/{sandboxID}/logsin spec; keeps existing behavior for backward compatibilityV1SandboxLogsnow supportsendanddirection; default forward/backward handling wired through to LokiQuerySandboxLogsnow acceptsdirectionand passes it to Lokisandbox_logs_v2.go): maps request params to edge call with a max 7-day window aroundcursorWritten by Cursor Bugbot for commit 7e0ef78. This will update automatically on new commits. Configure here.