Log referrer header at a configurable rate #182
Conversation
endpoints/get.go
Outdated
| @@ -38,9 +46,16 @@ func NewGetHandler(storage backends.Backend, metrics *metrics.Metrics, allowCust | |||
|
|
|||
| func (e *GetHandler) handle(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | |||
| e.metrics.RecordGetTotal() | |||
|
|
|||
| if utils.RandomPick(e.cfg.refererLogRate) == true { | |||
There was a problem hiding this comment.
The == true check isn't required.
There was a problem hiding this comment.
Still looks like it's there. Did you forget to save or commit?
There was a problem hiding this comment.
I don't know 😅 . Corrected
config/config_test.go
Outdated
| expectedLogInfo []logComponents | ||
| }{ | ||
| { | ||
| name: "invalid_nagative", // must be greater or equal to zero. Expect fatal log |
There was a problem hiding this comment.
Nitpick: Typo invalid_nagative -> invalid_negative
| {msg: "config.request_logging.referer_sampling_rate: 0", lvl: logrus.InfoLevel}, | ||
| {msg: "config.backend.type: memory", lvl: logrus.InfoLevel}, | ||
| {msg: "config.compression.type: snappy", lvl: logrus.InfoLevel}, | ||
| {msg: "Prebid Cache will run without metrics", lvl: logrus.InfoLevel}, |
There was a problem hiding this comment.
Nice simplification here.
endpoints/get.go
Outdated
| @@ -38,9 +46,16 @@ func NewGetHandler(storage backends.Backend, metrics *metrics.Metrics, allowCust | |||
|
|
|||
| func (e *GetHandler) handle(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | |||
| e.metrics.RecordGetTotal() | |||
|
|
|||
| if utils.RandomPick(e.cfg.refererLogRate) == true { | |||
There was a problem hiding this comment.
Still looks like it's there. Did you forget to save or commit?
utils/utils_test.go
Outdated
| }, | ||
| } | ||
| for _, tc := range testCases { | ||
| assert.Equal(t, tc.expected, RandomPick(tc.inPickProbability), tc.desc) |
There was a problem hiding this comment.
Can you use t.Run here too? .. and also change desc to name. Test names can be simplified to "zero" and "one". Thank you for not trying to test the random number generator part. :)
There was a problem hiding this comment.
Do you agree with naming this function RandomPick? I'm not so sure about the name but, I can't think of anything better of the top of my head.
There was a problem hiding this comment.
It doesn't bother me. Other options to consider:
ShouldSampleShouldLogRandomChance
| @@ -38,9 +46,16 @@ func NewGetHandler(storage backends.Backend, metrics *metrics.Metrics, allowCust | |||
|
|
|||
| func (e *GetHandler) handle(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { | |||
| e.metrics.RecordGetTotal() | |||
|
|
|||
| // If incoming request comes with a referer header, there's a e.cfg.refererLogRate percent chance | |||
There was a problem hiding this comment.
IMHO: You don't need these comments. You added them in this commit, is that related to the RandomPick name?
endpoints/get_test.go
Outdated
| uuid: "", | ||
| allowKeys: false, | ||
| uuid: "", | ||
| cfg: testConfig{allowKeys: false}, |
There was a problem hiding this comment.
Can you add two test cases where the referer log rate is 1.0 and the referer is specified in one test case and not specified in the other test case? It seems like that would be easy to do since this test already has logEntries as an expectation right?
There was a problem hiding this comment.
Good catch. Added those two unit tests.
utils/utils_test.go
Outdated
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| assert.Equal(t, tc.expected, RandomPick(tc.inPickProbability), tc.name) |
There was a problem hiding this comment.
You shouldn't need tc.name at the end of this assert statement since it is already passed in as the first parameter to t.Run.
| @@ -9,3 +11,13 @@ func GenerateRandomID() (string, error) { | |||
| u2, err := uuid.NewV4() | |||
| return u2.String(), err | |||
| } | |||
|
|
|||
| func RandomPick(pickProbability float64) bool { | |||
There was a problem hiding this comment.
Is there any value in injecting rand as a dependency as a second parameter of type interface to this function or by creating a RandomPicker struct with the dependency being a field and this function is a receiver method so that we can mock it for testing purposes? Is that overkill @SyntaxNode @guscarreon?
There was a problem hiding this comment.
Discussed offline. The extra work required to make this fully testable via unit test is not worth it due to the simplicity of the logic.
endpoints/put.go
Outdated
| // If incoming request comes with a referer header, there's a e.cfg.refererLogRate percent chance | ||
| // getting it logged | ||
| if referer := r.Referer(); referer != "" && utils.RandomPick(e.cfg.refererLogRate) { | ||
| logrus.Info("PUT request Referer header: " + referer) |
There was a problem hiding this comment.
Is this line being tested in put_test.go? I don't see coverage. Is it worth adding a test similar to my recommendation in get_test.go?
There was a problem hiding this comment.
Done. Added JSON tests on both endpoints
This pull request modifies PRebid Cache core so, when configure, it is able to output the value of incoming requests
Refererheader at a rate that can be configured inconfig.yamlorconfig.jsonor the environment variablePBC_REQUEST_LOGGING_REFERER_SAMPLING_RATE. The config value represents the probability of logging theRefererheader of a given request.The sampling method uses a golang-generated random number and, if set to say,
0.1, it will log approximately 10% of all incoming requestRefererheaders.