agent: Add scaling event reporting #1107
Conversation
No changes to the coverage.
HTML Report |
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-2
branch
from
693b601 to
a3cf0fa
Compare
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-1
branch
from
b70150d to
54bfb21
Compare
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-1
branch
from
f608569 to
1c71a57
Compare
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-2
branch
from
a3cf0fa to
16c0917
Compare
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-1
branch
from
a46466d to
df54b37
Compare
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-2
branch
from
16c0917 to
d2b4d45
Compare
This is part 2 of 2; see #1078 for the ground work. In short, this commit: * Adds a new package: 'pkg/agent/scalingevents' * Adds new callbacks to core.State to allow it to report on scaling events changes in desired CU.
sharnoff
force-pushed
the
sharnoff/scaling-event-reporting-2
branch
from
d2b4d45 to
8c60b7f
Compare
sharnoff
requested review from
a team and
Omrigan
and removed request for
a team
|
Remaining items for me, on this:
In the meantime, it should be ok to review. |
| // This exists because Neon allows fractional compute units, while the autoscaler-agent acts on | ||
| // integer multiples of a smaller compute unit. |
There was a problem hiding this comment.
I never paid attention to this difference before, I'd like to discuss it more broadly.
pkg/agent/core/state.go
Outdated
| ScalingEvent ReportScalingEventCallback | ||
| DesiredScaling ReportDesiredScalingCallback |
There was a problem hiding this comment.
Suggestion: maybe instead of having those as callbacks, define a new adapter interface, and pass it like this?
Passing interface feels more idiomatic.
There was a problem hiding this comment.
Hm, could you expand on this? (e.g., just these two? or the entire ObservabilityCallbacks struct?)
The reason I didn't already do that is because it seemed like a lot of boilerplate just to glue together different functionalities.
There was a problem hiding this comment.
I mean just this two.
I guess I am trying to figure out when it is better to have callbacks, and when it is better to introduce the adapter. Probably, what differentiates things I previously put into the ObservabilityCallbacks is that those are at the top of callstack, while you tend to use anonymous functions for DI, essentially gluing one piece of complex code to another piece of complex code. Plus, I tend to think about callbacks as "something people are forced to use in C, because they don't have an adequate method for defining methods on an object, and doing polymorphism"
But probably I am being too strict, the wiring is perfectly understandable either way. It is fine to use callbacks in Go, as long as we don't put massive amounts of business logic into it. And we even have anonymous functions, compared with C.
There was a problem hiding this comment.
That all makes sense. I think in general you're right to call out using an interface instead, especially because I have a tendency to make big anonymous functions 😅
In this case, I would say it matches this:
It is fine to use callbacks in Go, as long as we don't put massive amounts of business logic into it.
Going to resolve this as-is for now, but we can always revisit later if you like
Co-authored-by: Oleg Vasilev <[email protected]>
|
One comment thread left, cc @Omrigan |
|
Seems ready to merge (thanks for reviews!); I'm planning on testing on staging first. Two comments to unresolve after merging, for posterity: #1107 (comment) and #1107 (comment). |
|
Tested on staging; looks good! |
Follow-up to #1107, ref #1220. Basically, this change adds support into 'pkg/reporting' (which the autoscaler-agent uses for billing and scaling events) to cleanly send all remaining events on shutdown. This has three pieces: 1. The event sender threads now send and exit when their context expires; 2. reporting.NewEventSink() now takes a parent context (or .Finish() can be called instead to explicitly trigger a clean exit); and 3. reporting.NewEventSink() now spawns the senders with a taskgroup.Group passed in by the caller, so that shutdown of the entire autoscaler-agent will wait for sending to complete.
Follow-up to #1107, ref #1220. In short, this change: 1. Moves reporting.EventSink's event sender thread management into a new Run() method 2. Calls (EventSink).Run() from (billing.MetricsCollector).Run(), appropriately canceling when done generating events, and waiting for it to finish before returning. 3. Adds (scalingevents.Reporter).Run() that just calls the underlying (EventSink).Run(), and calls *that* in the entrypoint's taskgroup. Or in other words, exactly what was suggested in this comment: #1221 (comment)
Must have forgotten to include handling for azure blob storage clients as part of #1107; this change adds it. While we're at it, unify the config checking for azure blob between billing and scaling events (like we have for S3), and also start checking prefixInContainer where we weren't before.
Must have forgotten to include handling for azure blob storage clients as part of #1107; this change adds it. While we're at it, unify the config checking for azure blob between billing and scaling events (like we have for S3), and also start checking prefixInContainer where we weren't before.
This is part 2 of 2; see #1078 for the ground work and neondatabase/cloud#15939 for the full context.
In short, this PR:
pkg/agent/scalingeventscore.Stateto allow it to report on scaling events changes in desired CU.Notes for review:
I'd like to add minio-based S3 tests to this, but it seemed like it'd be non-trivial, particularly because scaling events actually require that there's scaling that happens — unlike the existing billing tests.
So I figured I'd open this for review in the meantime.
Also note: This PR builds on #1078 and must not be merged before it.