feat(metrics): Log the number of stored accounts on the /signin page. #4493
Conversation
| */ | ||
| logNumStoredAccounts () { | ||
| const numAccounts = Object.keys(this._accounts()).length; | ||
| this._metrics.logEventOnce(`num.accounts.${numAccounts}`); |
There was a problem hiding this comment.
I went with this format instead of a top level metric because top level metrics are reported on every flush and this minimizes the number of duplicate events.
There was a problem hiding this comment.
I wonder if it's worth bucketing this in case of outliers, e.g. log everyone with more than 2 accounts into a singlenum.accounts.two_or_more bucket.
There was a problem hiding this comment.
Yeah agreed, this event should either use the .histogram feature of node-statsd or bucket the numAccounts
There was a problem hiding this comment.
I'd prefer to make transformations as late as possible for flexibility and so we can visualize data in different ways in the future.
Yeah agreed, this event should either use the .histogram
We tried using .histogram way back in the day and had some issues, I forget what they were, but I vaguely remember .histogram being treated identically to .count items. Maybe that's been fixed, or there's an extremely high chance I'm mis-remembering. Can histograms be done with normal .count items in DD?
or bucket the numAccounts
I understand the case of outliers, though can the same be done in later stages (DD) so we still retain flexibility?
There was a problem hiding this comment.
We tried using .histogram way back in the day and had some issues, I forget what they were, but I vaguely remember .histogram being treated identically to .count items.
Your memory is correct. Forwarding you the email thread that we had about this with DataDog, although it was a while back (September 2015) so maybe things are different now.
|
from mtg: needs update to PR |
shane-tomlinson
force-pushed
the
log-number-of-stored-accounts
branch
from
c6789e3
to
aab0848
Compare
shane-tomlinson
changed the title
shane-tomlinson
force-pushed
the
log-number-of-stored-accounts
branch
from
aab0848
to
30d45e2
Compare
shane-tomlinson
changed the title
|
I'm happy with how this turned out, @rfk, and @vladikoff - mind having another r? |
| this._entrypoint = options.entrypoint || NOT_REPORTED_VALUE; | ||
| this._migration = options.migration || NOT_REPORTED_VALUE; | ||
| this._service = options.service || NOT_REPORTED_VALUE; | ||
| this._able = options.able; |
There was a problem hiding this comment.
I found it difficult to navigate this mess, so I sorted most items alphabetically.
| // by tag. Convert the count to a tag so we can see which tag has | ||
| // the most values. | ||
| context.increment( | ||
| type, ['has:' + reportedNumStoredAccounts].concat(tags)); |
There was a problem hiding this comment.
This is going to produce a tag like has:2, right? Will there be enough other context on the events to remember what that's about when looking at the graphs 6 months from now, or should we use a more descriptive tag?
There was a problem hiding this comment.
Will there be enough other context on the events to remember what that's about when looking at the graphs 6 months from now
This was my worry as writing the PR. I have no existential problems having a tag and event name being almost identical: event name: num_stored_accounts, tag name: num_stored_accounts:0, num_stored_accounts:1 or num_stored_accounts:2
@vladikoff - seem reasonable?
The User model became very complex to support allowing users to sign in to reliers using more than one address. This is to see how many accounts the normal user has whenever they first visit the signin page to see how often this capability is actually being used. If a very low number of people make use of this, I'd like to vastly simplify the User model.
Send `numStoredAccounts` as a top level metric from the client->server. The metric will normally only be sent once. When sending data to DataDog, increment a `num_stored_accounts` event and use 3 tags to bucket users, `has:0`, `has:1`, and `has:2`.
shane-tomlinson
force-pushed
the
log-number-of-stored-accounts
branch
from
30d45e2
to
0058549
Compare
The User model became very complex to support allowing users to sign in
to reliers using more than one address. This is to see how many accounts
the normal user has whenever they first visit the signin page to see
how often this capability is actually being used. If a very low number
of people make use of this, I'd like to vastly simplify the User model.
@vladikoff or @rfk - r?