Avoid double-counting the /sign_in page when users return from authenticating with their primary. #3971
Conversation
| @@ -12,6 +12,8 @@ const urlparse = require('urlparse'); | |||
|
|
|||
| // utility function to log a bunch of stuff at user entry point | |||
| module.exports = function(req, res, next) { | |||
| // NOTE - this only counts the first time the user sees the dialog, it does | |||
| // not count if the user returns from a primary. | |||
There was a problem hiding this comment.
oh right, because "/sign_in" !== "/sign_in?AUTH_RETURN". Took me a moment.
There was a problem hiding this comment.
I should probably say that in the comment, eh?
|
@shane-tomlinson : its an expansion of scope, but it would be useful to count the ?AUTH_RETURN case separately. In that case, the referrer is the IdP, which would give us a window on # of unique IdPs (and a heads up if new IdPs show up). |
|
@kparlante - once #3968 is merged, I'll complete this. I have most of it done, but want to use the testing from that PR to enable clean testing of parts of this PR. |
|
#3968 is merged, sorry I've been lagging recently 🍻 |
|
@shane-tomlinson OK, so my concern coming in was that the querystring would be visible to other servers, and maybe get cached. Looking at our caching behavior for wsapis and the HTTP 1.1 spec, I think we should be good. Does anything show up in the querystring that could be spoofed and cause problems? Trying to think through potential issues caused by that being visible to servers. |
| function entry(type, req, data) { | ||
| logger.info(type, _.extend({ | ||
| browser: req.headers['user-agent'], | ||
| // IP address (this probably needs to be replaced with the |
There was a problem hiding this comment.
maybe now is the time to finally remove this slightly misleading comment?
I think we are specifically not ever logging the real IP address, it's always 127.0.0.1, is it worthwhile to bother including it?
There was a problem hiding this comment.
@6a68 - on ephemeral instances, the real address is actually logged. For instance, this is what I see in router.log when running Selenium tests from Sauce:
{"browser":"Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0","ip":"199.27.108.155","rp":"http://logging-shane.123done.org","level":"info","message":"signin","timestamp":"2013-10-16T15:22:02.903Z"}
As you can see, I put the ip address stuff back in after seeing this. Note, we do NOT send the IP address off to kpiggybank.
|
@6a68 - this should be cached in the same way as I have removed the IP address from the logs since it was always 127.0.0.1 |
| @@ -37,7 +37,7 @@ const MetricsKpiggybankTransport = require('./transports/metrics-kpi'); | |||
| exports.logger.add(LogFileTransport, {}); | |||
| exports.logger.add(StatsdTransport, {}); | |||
| exports.logger.add(MetricsFileTransport, {}); | |||
| exports.logger.add(MetricsKpiggybankTransport, {}); | |||
| exports.logger.add(MetricsKpiggybankTransport.getInstance(), {}, true); | |||
There was a problem hiding this comment.
I use a singleton here so that I can get the same transport instance for testing.
|
This code is live on https://logging-shane.personatest.org |
…ticating with their primary. * Instead of using the #AUTH_RETURN* hash, use ?AUTH_RETURN*, which is sent to the backend and can be used to avoid double count. * /sign_in?AUTH_RETURN logs an `idp.auth_return` message * /sign_in?AUTH_RETURN_CANCEL logs an `idp.auth_cancel` message * Update tests. fixes #3970
|
I added a new commit to log new primary users, which addresses #3985 |
…as updated yesterday in dev.
| * complete_user_creation.success | ||
| * idp.auth_return | ||
| * idp.auth_cancel | ||
| * idp.create_new_user |
There was a problem hiding this comment.
this list doesn't seem to include metrics.report.*
There was a problem hiding this comment.
eh, if I'm correct, just add this in a tiny follow-on PR.
|
Looks good, tests pass. R+, merging. |
…le-count Avoid double-counting the /sign_in page when users return from authenticating with their primary.
I am slightly uncomfortable with this and am opening to get feedback from @6a68, @kparlante and @seanmonstar.
fixes #3970