Hook interfaces don't pass context

[brennan-airtime]

I see there is another issue that talks about an otel plugin. I don't know what the extent of that is but some thoughts after making my own proxy on top of kgo to add tracing.

I also understand the exact changes I'm suggesting would break the api, I'm more just thinking out loud.

type HookProduceRecordBuffered interface {
	OnProduceRecordBuffered(*Record)
}

If we take producing a record for example, the hook is not passed the context the record was published with which means it cannot be used for opentelemetry trace context or baggage propagation. If the hook were passed the Context the text map propagator could be used to serialize the trace context into the headers of the record before publishing.

Consuming is trickier as the api doesn't allow passing back a different context when the record is processed:

func (cl *Client) PollFetches(ctx context.Context) Fetches

But perhaps an api that is callback focused would make it possible and would require new hooks to help drive the callbacks to wrap them in tracing.

func (cl *Client) PollFetches(ctx context.Context, func (ctx context.Context, Fetches))
// But each message could be part of a different trace so really we need to invoke a callback per record
func (cl *Client) PollFetches(ctx context.Context, func (ctx context.Context, record *Record, err FetchError))

[brennan-airtime]

Oh I see an open PR that stuffs the context into the Record. That would work ok for producing.

On the consumer side it would still mean manually instrumented tracing to track processing the records but maybe that is ok. I don't have a good sense for how much of that would be heavily application specific.

[yianni]

@brennan-airtime , Here's how we propagate tracing context in the kotel plugin:

github.com/yianni/franz-go/blob/kotel-refactor/plugin/kotel/tracing.go

[brennan-airtime]

@yianni Ah ok so you're using the Ctx stuffed into the record.

Then I suppose to create the ${r.Topic} process spans I instrument that in my application code by pulling out the record.Ctx that was populated by the fetch hook. Which honestly feels about right given all the different ways I could choose to process a batch of fetched records.

[twmb]

I'll be merging #201 today (adding ctx to record), but 1.8 is still a bit off pending other features (I'm a bit slow for closing the loop on some of these)

[twmb]

Merged -- agreed with the discussion above, the fetch hook can be used to initialize / populate the record.Context and it's about the best way to do it.

I'm going to close this issue since there isn't anything to do here (I think, though please let me know if it should be reopened). 1.8 should be released soooonish...

[brennan-airtime]

Nah I'm all good. This was mostly a "see something say something" issue as I was working on using the library.

It looks like putting the context in the record is the easiest way to not have to change the entire api or bloat the api surface area.

And it wouldn't be too bad in the caller.

fetches := kc.PollFetches(ctx)
fetches.EachRecord(func (record *kgo.Record) {
  ctx, span := tracer.Start(record.Ctx, fmt.Sprintf("%s process", record.Topic))
  defer span.End()
  // ... do work
})

@yianni It may be nice for your kotel package to expose code that can fill in a span with the standard attributes from a record.