Alerting: Use a completely isolated context for state history writes

[alexweav]

What is this feature?

The background tasks to write state history data currently pass along the main rule evaluation context.

It doesn't make sense to do this. These writes are totally different background jobs. Passing the context along here means that state history writes might inherit polluted context, unrelated timeouts, and other things.

In this PR, we spin up a brand new context for state history writes, and seed it with the things we expect. I analyzed the existing contexts, which contained three things consistently:

• Timeouts - I saw many pre-existing timeouts from other areas. These should not be passed along, this is a bug. Replaced with a fresh timeout just for state history.
• Rule data - used for logging. Added it to the new context.
• Tracing context - Right now, state history writes are not visible on the spans we emit anyway. I did not bother to pass these through, since we already aren't getting this data, so we aren't losing anything by dropping it. Tracing for these jobs would be nice admittedly - but adding this is better suited for a future PR.

Which issue(s) does this PR fix?:

n/a

Special notes for your reviewer:

[gotjosh]

Fixes #64979

[yuri-tceretian]

Tracing context - Right now, state history writes are not visible on the spans we emit anyway. I did not bother to pass these through, since we already aren't getting this data, so we aren't losing anything by dropping it. Tracing for these jobs would be nice admittedly - but adding this is better suited for a future PR.

I don't think this is the correct statement. Last time I checked when I introduced the tracking in scheduler, db queries initiated in historian were included. If that stopped happening, we should fix that.

[alexweav]

I don't think this is the correct statement. Last time I checked when I introduced the tracking in scheduler, db queries initiated in historian were included.

I've checked maybe 10 or 15 spans in Tempo and I'm not seeing this at all. Perhaps I'm looking in the wrong place?

If that stopped happening, we should fix that.

Agree, but I'd prefer a separate PR as this change doesn't regress. Shall I file an issue?

[yuri-tceretian]

What do you think about sending data to historian via a channel. This way we will make it an independent service, decouple and make that channel buffer be a backpressure mechanism (i.e. not need for timeouts on client side)

[alexweav]

I think that seems like a nice goal. We'd at the minimum get a little more control over pileups. It's a fairly deep change to implement this right now because we still pass pointers to states in the registry everywhere and not copies, so the states could change underneath it while it sits in the queue. This would need to be fixed first, i.e. doing for state what we already did for rule.

I still think the consumer of the queue should have timeouts though - especially since it's talking to an external service. At a minimum it would need its own context anyway, so even if we start moving toward that goal, this PR is still needed first.

[gotjosh]

LGTM but please see my comment regarding your comment 😆

[alexweav]

another approach here would be to just detect the span type and inject appropriately, avoiding need to extend the Span interface.

IMO the current approach is better decoupled, but curious what others think?

[sakjur]

Generally speaking I prefer public methods and casting to other interfaces for optional functionality over having hidden methods for cases like these. Otoh, we'll most likely revisit this when OpenTracing finally gets removed, so lets not overthink it — we can adjust the implementation over time.

[sakjur]

Visual review of the changes to pkg/infra/tracing: Looks good to me 🙂

[yuri-tceretian]

LGTM