ruler: Support writeNotify on Loki ruler

[kavirajk]

What this PR does / why we need it:
prometheus ruler added a feature of notifying the reader when a sample is appended, instead of waiting in a loop burning the CPU cycles. prometheus/prometheus#11949

This changes a default behaviour a bit. Now if notify is not enabled, next read is done only when next readTicker is triggered.

Which issue(s) this PR fixes:
Also should fix #10859

Special notes for your reviewer:
Adding few more details for the sake of completeness.

We found this via more frequent failures of rule-evaluation integration tests linked on the issue above. After some investigation, we tracked down to prometheus changes.

Prometheus introduced new type wlog.WriteNotified interface with Notify() method with a goal to notify any waiting readers, that some write is done.

Two types implements this type wlog.Watcher and remote.Storage. remote.Storage implements Notify() by just calling it's queues wlog.Watcher's Notify() under the hood.

How are these types impacts Loki ruler?

Loki ruler also uses remote.Storage. So when any samples got committed via appender, we have to notify the remote storage.

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• CHANGELOG.md updated
  • If the change is worth mentioning in the release notes, add add-to-release-notes label
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

[dannykopping]

This is a little clunky; how about just passing a Notify() function to the appender? The appender doesn't need any knowledge of the wlog.WriteNotified.

[jeschkies]

I wouldn't have thought about this fix. This is cool. That said I'm a little confused. Shouldn't we implement Notify() and pass it on?

[kavirajk]

Agree on the clunkiness :)

It comes from

type appender struct {
	w             *Storage
	writeNotified func() wlog.WriteNotified
	series        []record.RefSeries
	samples       []record.RefSample
	exemplars     []record.RefExemplar
}

writeNotified being func() type.

It got bit tricky because we initialize few things during NewStorage() and few other things during initialize() call.

wal is created during wal.NewStorage but the remoteStorage (that has Notify()) only created during initialize. We have to it as func type because we can only know if wal.Storage has write notify set that way. Also note, wal.Storage uses appender as a pool.

We can make it more clean by refactoring few things on NewStorage and initialize. But AFAIK not worth it now?

[dannykopping]

In SetWriteNotified (nit: I think it should be SetWriteNotifier, by the way, it's a pity the prom type is named weirdly), you could just reference the Notify function there, no?

You could then have something like this:

type appender struct {
	w             *Storage
	notify        func()
	series        []record.RefSeries
	samples       []record.RefSample
	exemplars     []record.RefExemplar
}

func (w *Storage) SetWriteNotifier(notifier wlog.WriteNotified) {
        if notifier == nil { return }

	w.notifier = notifier.Notify
}

	// Notify so that reader waiting for it can read without needing to wait for next read ticker.
	if a.notify == nil {
           // log unexpected nil notifier
	}

        a.notify()

[kavirajk]

We can make notify as just func(). Agree. But cannot be done quite like you mentioned here. Given relationship between wal.Storage and appender is via sync.Pool.

I made it work, by moving some things to appenderPool.New func. Seems better now and solves your original concern.

[kavirajk]

Also about the nit: SetWriteNotified -> SetWriteNotifier.

I think they calling it WriteNotified and not WriteNotifier because, WriteNotifier is confusing in a sense, is it notifying the "write" that is done? or notifying the "writer".

We want to notify the "write" that is actually done (potentially to readers waiting for it).

[dannykopping]

Almost there, thanks!

[dannykopping]

LGTM, one small grammar nit