1246728 - Domain collection endpoint

Status: RESOLVED FIXED

(Hello (Loop) :: Server, defect, P1)

Description

[Ian Bicking (:ianbicking)]

For Bug 1211542 we want to collect some domain names on the server.

I propose we add a new endpoint to the server, say... /log-domains.  This would take a POST request with a JSON body, the body would look like {"google.com": 4, "other": 10}.  We would then emit log messages with this data, to later be picked up by our server metrics.  (We might explode a message like this into {"domain": "google.com", "count": 4} and emit multiple log messages per request)

Comment 1

[Ian Bicking (:ianbicking)]

Remy suggests we should require the standard authentication on this endpoint to make it harder for third parties to spam this logging.

Comment 2

[Rémy Hubscher (:natim)]

Ian could you provide me with the list of available domains?

I would like to make sure the given domain is acceptable. It could also be a generic regexp.)

Do you want to handle some subdomains as well?

Comment 3

[Rémy Hubscher (:natim)]

Attachment: First step is to use a tag aware statsd client.

A first PR to use tags for faceting stats counters.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8718837 [details] [review]
First step is to use a tag aware statsd client.

As commented on github:

The patch looks good to me with some nits here and there, so from a code standpoint and as the tests are green, r+wc. 

Note: I know basically nothing about the Loop production server infrastructure and its constraints, but I wonder about a possible backward compatibility issue with the renamed log events though.

Comment 5

[Rémy Hubscher (:natim)]

What do you think about adding a new ``logDomain`` action to the room as we did for status [0].

We could benefit for checking the user is authenticated as well as present in the room we are logging domains for.

[0] https://github.com/mozilla-services/loop-server/blob/master/loop/routes/rooms.js#L486-L501

Comment 6

[Rémy Hubscher (:natim)]

After talking with Bob, it seems that tags cannot be used with up to 2000 domains names, it will be too much.

It would be better to log things to heka that way:

{
   "domain": "ebay.com",
   "count" : 2,
   "domain-counter" : true
}

Also because we cannot have nested JSON this makes it more difficult to log into Heka.
I will investigate that next week.

Comment 7

[Rémy Hubscher (:natim)]

Attachment: LogDomain feature for Loop-Server

Comment 8

[Bob Micheletto [:bobm]]

(In reply to Rémy Hubscher (:natim) from comment #6)

> {
>    "domain": "ebay.com",
>    "count" : 2,
>    "domain-counter" : true
> }

I've run this by a Data Engineer, and have been informed that I got the formatting on that somewhat wrong.  In the mozlog format, minus the usual Timestamp and other fields the library should populate for you, the format should look like so:

{
   "Fields" : {
      "MyFavoriteDomain.blah" : 4,
      "miscellaneous" : 10
   },
   "Type" : "domain-counter"
}

Type is the mozlog place to define the type of record, and should replace the domain-counter: true.
Fields is the mozlog place to put application data.

Comment 9

[Bob Micheletto [:bobm]]

Comment on attachment 8719520 [details] [review]
LogDomain feature for Loop-Server

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1246728#c6

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8719520 [details] [review]
LogDomain feature for Loop-Server

The code looks okay to me, but one more time I don't have much context about our metrics infrastructure to ensure it matches it and properly serves the intended purpose. So r+ conditional to other reviews from people from metrics.

Comment 11

[Rémy Hubscher (:natim)]

https://github.com/mozilla-services/loop-server/commit/817a5a8618eb4c332e0ff0b9290f7b1b38e8b474

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8718837 [details] [review]
First step is to use a tag aware statsd client.

Guessing these are obsolete requests given this bug is closed now.