Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

event: add max eps reporting. #515

Merged
merged 1 commit into from Nov 16, 2018
Merged

event: add max eps reporting. #515

merged 1 commit into from Nov 16, 2018

Conversation

AlexJF
Copy link

@AlexJF AlexJF commented Nov 6, 2018

This PR adds periodic max eps sampling reporting. This reporting is 2-fold:

  • Statsd gauges for current, max, sample_rate and max_reached.
  • Log lines prompting the user to tune the MaxEPS setting if the maximum event rate was reached.

Sample output:

=== RUN   TestEventProcessorFromConf/metric_-_above_max_eps
1541500869865907000 [Warn] Max events per second reached (current=193.66/s, max=100.00/s). Some events are now being dropped (sample rate=0.52). Consider increasing MaxEPS setting.
1541500899864986000 [Warn] Max events per second reached (current=224.48/s, max=100.00/s). Some events are now being dropped (sample rate=0.45). Consider increasing MaxEPS setting.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I think this resulted in being a bit over-engineered in an attempt to fit in this decorator pattern which is not natural to Go imho. The fact that a type cast is required and that the new methods added on the sampler say nothing about their use (for stats) is a good indication that this isn't the way to do it.

I'd prefer it if we didn't go with this solution. The intention here is to only report occasionally, right?

Instead, let's:

  • Add a method (*maxEPSSampler).report which reports to statsd when called.
  • Inside (*maxEPSSampler).Start create a new routine which calls that method on an interval:
go func() {
    for range s.reportTicker.C {
        s.report()
    }
}()
  • Call reportTicker.Stop in (*maxEPSSampler).Stop

In my opinion this is the way to go. This way, instrumentation is separated out into its own method as you've intended, while still not decoupled from its source. At this stage, we don't need a reusable component. Even so, if we were to create one we should do it differently.


If we need a reusable component later, we could create an interface:

type Reporter interface {
    Report(client statsd.Client)
}

We could then simply add this method to the sampler (or any other component) to enable it as a candidate for interval reporting. A separate struct could use these reporters to call them on an interval. This way the implementation and intention is clear, with not much modification required.

@gbbr gbbr changed the title Add max eps reporting. event: add max eps reporting. Nov 6, 2018
@gbbr
Copy link
Contributor

gbbr commented Nov 6, 2018

Edit: the for loop for the ticker in my example needs to be extended a bit, more along the lines of the example in https://golang.org/pkg/time/#NewTicker to exit cleanly.

@AlexJF AlexJF force-pushed the alexjf/warn-maxeps branch 3 times, most recently from 3f9a97c to c49fce6 Compare November 6, 2018 13:47
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks Alex. LGTM. Left a couple more nits but don't feel strongly about them so I'll leave it up to you.

func NewMaxEPSSampler(maxEPS float64) Sampler {
return newMaxEPSSampler(maxEPS, newSamplerBackendRateCounter())
// NewMaxEPSSampler creates a new instance of a MaxEPSSampler with the provided maximum amount of events per second.
func NewMaxEPSSampler(maxEPS float64, reportFrequency time.Duration) *MaxEPSSampler {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the reportFrequency from the arguments list, since it's not likely to be needed there, right? Then, for testing, you can keep it in the unexported constructor. It could be just a const.

@@ -47,19 +75,51 @@ func (s *maxEPSSampler) Sample(event *model.APMEvent) (sampled bool, rate float6
return true, 1
}

Copy link
Contributor

Choose a reason for hiding this comment

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

With the risk of becoming annoying here: can we remove all the whitespace?

event/sampler_max_eps.go Outdated Show resolved Hide resolved
@AlexJF AlexJF force-pushed the alexjf/metric-based-event-extractor branch from 8ac92b4 to 57be144 Compare November 13, 2018 09:56
@AlexJF AlexJF added this to the 6.7.0 milestone Nov 13, 2018
event/sampler_max_eps.go Outdated Show resolved Hide resolved
event/sampler_max_eps.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

LGTM

event/sampler_max_eps.go Outdated Show resolved Hide resolved
event/sampler_max_eps.go Outdated Show resolved Hide resolved
@AlexJF AlexJF changed the base branch from alexjf/metric-based-event-extractor to master November 16, 2018 09:00
@gbbr gbbr merged commit ec88279 into master Nov 16, 2018
@gbbr gbbr deleted the alexjf/warn-maxeps branch November 16, 2018 09:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants