Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument beat output client #17274

Closed
wants to merge 3 commits into from

Conversation

graphaelli
Copy link
Member

@graphaelli graphaelli commented Mar 26, 2020

What does this PR do?

Instruments beats publishing output and the elasticsearch output. Other outputs will follow.

regular transaction image transaction metadata image
transaction with errors image
error list image error details image

Why is it important?

To gain insight into performance of libbeat based applications.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Run any beat with ELASTIC_APM_ACTIVE=true set in the environment. Examples here were generated with:

ELASTIC_APM_ACTIVE=true filebeat -e -E filebeat.inputs='[{type: stdin, enabled:true}]' 

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)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a hunch, but it looks like we potentially have a race condition between getTracer and SetTracer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need another local 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@graphaelli
Copy link
Member Author

closing for now to prevent notifications for every push

@graphaelli graphaelli closed this Apr 2, 2020
@jalvz jalvz mentioned this pull request Apr 23, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants