Refactor UnitHook to block the scan if finished metrics aren't handled

[mcastorina]

Description:

UnitHook was originally designed to drop metrics when the producer out-paced the consumer. This changes it so the hook will block the scan until there is room to write the finished metrics to the output channel.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rosecodym]

This code looks good (to my relatively inexpert eye), but do we have a monitoring plan yet? I'd like to avoid getting too out over our skies on the code itself.

[rosecodym]

Was this being done here before because there wasn't really a better place for it? But now, there is, so you moved it? Or am I misunderstanding?

[mcastorina]

This was the previous mechanism to drain the cache, which is now in EndUnitChunking and Finish. The idea was that you would periodically call UnitMetrics() and get the list of in progress and finished metrics, and the finished ones would be considered "handled" and removed from the cache.

This PR moves the handling to be synchronous, so this function is now only getting a snapshot of the in-progress units (hence the name change).

[rosecodym]

what's the difference between context.TODO() and context.Background()?

[mcastorina]

Functionally they're equivalent, but the docs say this:

Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter).

[rosecodym]

❤️

[ahrav]

This looks overall good to me. Do we have any way to know if we every actually run into the situation where the back pressure actually gets applied? It would be surprising, but nonetheless might be useful.

[mcastorina]

Do we have any way to know if we every actually run into the situation where the back pressure actually gets applied?

I could add some code where we write to the channel that would look like:

select {
    case u.finishedMetrics <- metric:
        // no backpressure
    default:
        // backpressure; do something / log?
        u.finishedMetrics <- metric
}

What do you think? Or maybe add another prometheus metric to track the size of the channel?

[ahrav]

Do we have any way to know if we every actually run into the situation where the back pressure actually gets applied?

I could add some code where we write to the channel that would look like:

select {
    case u.finishedMetrics <- metric:
        // no backpressure
    default:
        // backpressure; do something / log?
        u.finishedMetrics <- metric
}

What do you think? Or maybe add another prometheus metric to track the size of the channel?

Yea I think even just a log honestly would be great. I'm pretty curious to see if we ever execute that code path. I'd actually maybe want to get a profile if that did happen.

[mcastorina]

@rosecodym @ahrav I added both a log and a metric, and we can decide to keep one or the other later. Let me know what you think of the implementation.

[rosecodym]

Why does this function use metric (singular) but all of its call sites use metrics (plural)?

[mcastorina]

Indecisiveness lol. I tend to use plural for slices of objects, but for some reason I named the type plural UnitMetrics. 🤷

I'll make it plural for consistency, but open to suggestions in general for naming things.

[rosecodym]

Is this used anywhere?

[mcastorina]

Yes, it gets called when the SourceManager is waited:

trufflehog/pkg/sources/source_manager.go

Lines 173 to 177 in fb230aa

	for _, hook := range s.hooks {
	if hookCloser, ok := hook.(io.Closer); ok {
	_ = hookCloser.Close()
	}
	}

[mcastorina]

Some synchronization is required to make the goroutine reading the channel finish before everything gets shut down, but that doesn't happen in this PR since UnitHook isn't used yet.

I figured it made sense for the source manager to close out the hooks since the hook initialized it.

[rosecodym]

Some synchronization is required to make the goroutine reading the channel finish before everything gets shut down, but that doesn't happen in this PR since UnitHook isn't used yet.

You read my mind :)