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

Some email metrics are going to be broken in train 123 #2675

Closed
philbooth opened this issue Oct 23, 2018 · 0 comments · Fixed by #2676
Closed

Some email metrics are going to be broken in train 123 #2675

philbooth opened this issue Oct 23, 2018 · 0 comments · Fixed by #2676
Assignees
Labels

Comments

@philbooth
Copy link
Contributor

While testing the final fix for #2496, I noticed that the email metrics rely on a flowId and flowBeginTime that are manually copied from the request payload instead of via metricsContext.gather, which checks both the payload and memcached. This means that, with the content server stopping sending metrics context data to some endpoints in train 123, there would be a blip in the metrics.

I am working on a fix now, but paused to open this issue because the fix has led me to a wider refactoring of metricsContext.gather. Essentially, I intend to change it so we have a lazy getter for metrics context data on the request object. This has the mild benefit of stopping us from hitting gather multiple times per request as we do now (mild because memcached is fast) and the more significant benefit of getting rid of the slightly awkward interface for gather, where it mutates its input rather than returns fresh state.

It's not a hugely messy change, but it's big enough that I thought there should be a proper issue detailing the rationale, for us to refer back to in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant