Skip to content

fix(analytics): guard long-running sandboxes and set proper report timeout #1686

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.

Sign up for GitHub

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

Open
yaninyzwitty wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(analytics): guard long-running sandboxes and set proper report timeout #1686

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
034ca51
fix: improve long-running sandbox detection and adjust analytics timeout
yaninyzwitty Jan 6, 2026
991f418
fix: optimize long-running sandbox detection by simplifying time comp…
yaninyzwitty Jan 7, 2026
a4a8314
fix: optimize instance ID collection in analytics for long-running sa…
yaninyzwitty Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions packages/api/internal/orchestrator/analytics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,12 @@ func (o *Orchestrator) reportLongRunningSandboxes(ctx context.Context) {
case <-ticker.C:
sandboxes := o.sandboxStore.Items(nil, []sandbox.State{sandbox.StateRunning})
longRunningSandboxes := make([]sandbox.Sandbox, 0, len(sandboxes))
now := time.Now()
for _, sandbox := range sandboxes {
if time.Since(sandbox.StartTime) > oldSandboxThreshold {
if sandbox.StartTime.IsZero() {
continue
}
Copy link
Member

@jakubno jakubno Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check isn't really needed

if now.Sub(sandbox.StartTime) > oldSandboxThreshold {
Copy link
Member

@jakubno jakubno Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep

time.Since(sandbox.StartTime) > oldSandboxThreshold

I think it's more readable and the improvement is negligible

longRunningSandboxes = append(longRunningSandboxes, sandbox)
}
}
Expand All @@ -57,14 +61,14 @@ func sendAnalyticsForLongRunningSandboxes(ctx context.Context, analytics *analyt
return
}

childCtx, cancel := context.WithTimeout(ctx, syncAnalyticsTime)
childCtx, cancel := context.WithTimeout(ctx, reportTimeout)
defer cancel()

instanceIds := make([]string, len(instances))
executionIds := make([]string, len(instances))
for idx, i := range instances {
instanceIds[idx] = i.SandboxID
executionIds[idx] = i.ExecutionID
instanceIds := make([]string, 0, len(instances))
executionIds := make([]string, 0, len(instances))
Copy link
Member

@jakubno jakubno Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know the length will be exactly instances, so it's seems to be safe to preallocate and do it as it is now. What was the reason behind this change?

Copy link
Contributor Author

@yaninyzwitty yaninyzwitty Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well ok — given your current logic always appends exactly one entry per instance, this change doesn’t add much value. I will revert it to keep the code simpler.

for _, i := range instances {
instanceIds = append(instanceIds, i.SandboxID)
executionIds = append(executionIds, i.ExecutionID)
}

_, err := analytics.RunningInstances(childCtx,
Expand Down
Loading