Page MenuHomePhabricator

Graphoid Code Review
Closed, ResolvedPublic

Description

  • line 8: vega should be required directly; if it cannot be loaded, the whole service basically becomes unusable, so it seems useless to even start the service without it
  • line 33: putting : at the end of the defaultProtocol variable is highly unusual. It is not part of the protocol. Later, on line 198 // is appended to it, so that's the good place to put : as well. This also allows you to get rid of the hack on line 397
  • line 50: use sUtil.HTTPError instead of Err, as it automatically generates the response and whitelists fields that are to be sent, so you can still put anything you'd like logged.
  • line 343: better to return the failOnTimeout promise, so that the system may detect any uncaught errors and handle them gracefully.

A general note about logging: instead of using app.logger.log() please use the logger provided with the request object, as noted here. This allows us to track requests across different services, which is especially useful if Graphoid ends up behind RESTBase.

Event Timeline

mobrovac created this task.Apr 8 2015, 12:01 PM
mobrovac assigned this task to Yurik.
mobrovac raised the priority of this task from to High.
mobrovac updated the task description. (Show Details)
mobrovac added a subscriber: mobrovac.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 8 2015, 12:01 PM

Change 202910 had a related patch set uploaded (by Yurik):
Refactored logging and a few other things per review

https://gerrit.wikimedia.org/r/202910

Yurik added a comment.Apr 8 2015, 8:26 PM

I have fixed most of the issues, but I am hesitant to migrate to HTTPError just yet - it provides a number of good features, but it also misses things I am using, like metrics counts and non-error logging. I will review it separately and possibly migrate to HTTPError later.

Change 202910 merged by Yurik:
Refactored logging and a few other things per review

https://gerrit.wikimedia.org/r/202910

I have fixed most of the issues, but I am hesitant to migrate to HTTPError just yet - it provides a number of good features, but it also misses things I am using, like metrics counts and non-error logging. I will review it separately and possibly migrate to HTTPError later.

You are right, some general metrics logic in case of errors should exist. How about:

throw new HTTPError({
  message: 'blah blah',
  status: 500,
  metrics: {
    name: 'my.metric',
    func: 'endTiming',
    arg: startTime
  }
});

The expected result would be that when that error is caught, it would call endTiming('my.metric', startTime). Does that seem satisfactory?

What do you refer to when you say and non-error logging ?

Yurik added a comment.Apr 8 2015, 8:42 PM

I don't think bad user input should be logged as an error, but http code should be set to 4xx. Error is a bug in the service.
As for endTiming - a bit too verbose, for a large service you would have too many of them, obscuring the main logic... Not sure how to approach it yet, lets move it to a separate non-blocking discussion/service improvement.

Yurik added a comment.EditedApr 8 2015, 8:55 PM

Btw, re default protocol - the 'url' lib also treats colon as part of the protocol:

> require('url').parse('http://example.org').protocol
'http:'
mobrovac closed this task as Resolved.Apr 8 2015, 9:02 PM

Not sure how to approach it yet, lets move it to a separate non-blocking discussion/service improvement.

Fair enough. Closing as resolved. Thnx @Yurik !

mobrovac set Security to None.