Instrument beat output client #17274
Conversation
and some basics for the elasticsearch output
also makes transaction end more resilient
| tracer, err := apm.NewTracer(beat.Beat, beat.Version) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Would it be possible to pass the tracer to the constructor? Is possible I'd prefer to not have a 'SetTracer' and 'getTracer' method. Maybe add it to the 'Monitors' type?
| func (p *Pipeline) SetTracer(tracer *apm.Tracer) error { | ||
| p.tracer = tracer | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Just a hunch, but it looks like we potentially have a race condition between getTracer and SetTracer.
There was a problem hiding this comment.
certainly, this is just poc - the tracer would have to be set before publishing started like SetACKHandler
| @@ -78,6 +81,8 @@ type ClientConfig struct { | |||
| // ACKLastEvent reports the last ACKed event out of a batch of ACKed events only. | |||
| // Only the events 'Private' field will be reported. | |||
| ACKLastEvent func(interface{}) | |||
|
|
|||
| Tracer *apm.Tracer | |||
| @@ -34,7 +36,7 @@ type Client interface { | |||
| // Using Retry/Cancelled a client can return a batch of unprocessed events to | |||
| // the publisher pipeline. The publisher pipeline (if configured by the output | |||
| // factory) will take care of retrying/dropping events. | |||
| Publish(publisher.Batch) error | |||
| Publish(context.Context, publisher.Batch) error | |||
There was a problem hiding this comment.
This is a big change. When passing a context, is it only for tracing, or do we expect Deadline, Done, and Err to be used as well? In the later case error handling might need to be improved further to handle/ignore context.Cancelled. E.g. we do not want use Batch.Retry (decrease counter and eventually drop) on cancel, but Batch.Cancelled (keep TTL counter and rescheduled batch with another output).
There was a problem hiding this comment.
I had not intended to introduce cancellation/deadlines/etc as part of this change, but this could enable that as part of a separate effort.
There was a problem hiding this comment.
Not sure how feasible this would be :)
In this case can you please update the code comment to clarify the usage of the context passed?
|
closing for now to prevent notifications for every push |
What does this PR do?
Instruments beats publishing output and the elasticsearch output. Other outputs will follow.
Why is it important?
To gain insight into performance of libbeat based applications.
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
Run any beat with
ELASTIC_APM_ACTIVE=trueset in the environment. Examples here were generated with: