Skip to content

Conversation

@yaninyzwitty
Copy link
Contributor

  • Add check for zero StartTime to avoid misclassifying sandboxes as long-running
  • Use a single now timestamp for consistent evaluation within each tick
  • Replace syncAnalyticsTime with reportTimeout for analytics request context
  • Preallocate slices safely and append IDs instead of using full-length slice
  • Improves correctness, testability, and avoids sending empty or duplicate analytics events

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

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

Comment on lines 43 to 45
if sandbox.StartTime.IsZero() {
continue
}
Copy link
Member

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

Comment on lines 63 to 64
instanceIds := make([]string, 0, len(instances))
executionIds := make([]string, 0, len(instances))
Copy link
Member

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants