Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visualises requests when in development. #104

Closed
wants to merge 8 commits into from
Closed

Visualises requests when in development. #104

wants to merge 8 commits into from

Conversation

rickharrison
Copy link

I took a stab at visualising requests from #33. The one thing that is a little messy is parsing and displaying the json. I ended up keeping a reference to it on the request since you can only call getRawBody once per request.

Let me know your thoughts!

screen shot 2016-10-08 at 11 51 34 pm

@@ -37,6 +55,12 @@ async function run(req, res, fn, onError) {

async function json(req, {limit = '1mb'} = {}) {
try {
if (DEV && req.parsedJson) {
return req.parsedJson
} else if (DEV && req.jsonError) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we first check if there is an error? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, there can only be one or the other, but I can switch the order if you like.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine 👍, maybe add in a way to pass a custom parse function later on, so you can also log out other stuff. urlencoded forms for example.
For now this will do fine 😄

@rauchg
Copy link
Member

rauchg commented Dec 9, 2016

This is really beautiful @rickharrison. I think we should have --log=dev|prod|none as well that ignores env sniffing.

The prod mode of this would be a compressed version that excludes the body.

@rauchg
Copy link
Member

rauchg commented Dec 9, 2016

(basically something similar to the default express logger)

@rickharrison
Copy link
Author

Sounds good! I will work on this over my holiday break :)

@luisrudge
Copy link

This is amazing

@rickharrison
Copy link
Author

Here is what --log=prod looks like:

screen shot 2016-12-26 at 5 40 51 pm

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

@timneutkens
Copy link
Member

@rauchg final check?

@rauchg
Copy link
Member

rauchg commented Jan 6, 2017

I'll take a look!

-H Host to listen on [0.0.0.0]
-p Port to listen on [3000]
-h Show this help message
--log dev|prod|none`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off would be better than none IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need long forms for all of this, in addition to the short form

-H, --host
-p, --port
-h, --help
-L, --log

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add your second comment to my readme PR @rauchg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rauchg PR'ed #122

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -5,8 +5,22 @@ const server = require('http').Server
const getRawBody = require('raw-body')
const typer = require('media-typer')
const isStream = require('isstream')
const chalk = require('chalk')
const jsome = require('jsome')

const DEV = 'development' === process.env.NODE_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be cool to document in the README that we do the nicer logging upon explicit NODE_ENV = development setting

@@ -41,6 +63,14 @@ async function run(req, res, fn, onError) {

async function json(req, {limit = '1mb'} = {}) {
try {
if (req.jsonError) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid polluting req if at all possible

@@ -250,6 +250,14 @@ function handleErrors (fn) {
}
```

### Logging

Micro provides some built-in logging to help visualise requests. When `NODE_ENV` is set to `development`, micro will display the request along with any json body like this:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove some

@@ -28,7 +29,8 @@ const help = () => {
console.log(`Usage: micro [opts] <file>
-H, --host Host to listen on [0.0.0.0]
-p, --port Port to listen on [3000]
-h, --help Show this help message`);
-h, --help Show this help message
-L, --log dev|prod|off`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned this around in my merge commit. First the logging then help, also added [off] as default indicator

@timneutkens
Copy link
Member

@rauchg :shipit: 🚀

@rauchg
Copy link
Member

rauchg commented Jan 8, 2017

@rickharrison @timneutkens I think a few additions would make sense:

  • Response time (+ delta). EG: 10:30:00 PM (+30ms). Kinda like how debug gives you deltas
  • Also visualize response objects?
  • Not 100% sure, but I believe we can anchor the time to the right hand side with an ANSI escape code? (that'd be neat if possible!)

@rickharrison
Copy link
Author

Sounds good! I'll take a look at this later tonight.

@rickharrison
Copy link
Author

rickharrison commented Jan 8, 2017

@rauchg So I have it logging response objects as well now, but I did it by adding a property to res 🙁

The problem is I'm not sure how to get the response body in the res.on('finish') event. Is it possible to see the data chunks from a http.ServerResponse? An alternative would be to use the callback parameter in the res.end() call. However, I do not know what the current log level is in that method. Do you think it would be better to store the log level in a global variable like DEV is from the initial serve call? Then instead of attaching to res, we could output the response body in the res.end()

As another caveat, if send or a return value is not used, then the logger won't be able to pick up on the response body at all unless I am missing something and getting the data from the ServerResponse is possible.

I was unable to find a way to align the time text to the right side. If anyone else has an idea of how to do that with ANSI, please let me know.

Happy to chat more on the zeit slack about this as well!

EDIT: Updated screenshot

screen shot 2017-01-08 at 12 20 35 am

@rauchg
Copy link
Member

rauchg commented Jan 8, 2017

that looks really amazing! One way we could capture the response I assume is by patching the internal write method of the response object? The one that both res.end and res.write call

@rickharrison
Copy link
Author

Yea, we definitely could do that. I can take a stab and patching those methods inside of the logRequest method.

@rauchg
Copy link
Member

rauchg commented Jan 8, 2017

Also, I have a slightly different take on this now:

{
  "dependencies": {
     "micro": "latest"
  },
  "devDependencies": {
     "micro-dev": "latest"
  }
}
  • micro-dev would have micro in peerDependencies, rather than include it.
  • It works with the exact same signature as micro, but…
  • We can add more options related to logging, for example whether to include headers, if you want to log only requests or only responses, etc.
  • We get rid of NODE_ENV sniffing. If you run micro-dev, it logs.
  • Works really nicely for package.json. start can run micro, dev can run micro-dev

This way, micro remains lean both in download time, LOC/maintainability & performance!

@rauchg
Copy link
Member

rauchg commented Jan 8, 2017

@rauchg rauchg closed this Jan 8, 2017
@onbjerg
Copy link
Contributor

onbjerg commented Jan 20, 2017

@rauchg The repository is empty -- forgot to push or do you need a PR? 😊

@timneutkens
Copy link
Member

I'm working on it @onbjerg 😄

@onbjerg
Copy link
Contributor

onbjerg commented Jan 22, 2017

@timneutkens I couldn't wait so I implemented it as a simple wrapper function instead of a CLI tool: https://github.com/onbjerg/micro-visualize

@timneutkens
Copy link
Member

@onbjerg micro-dev is most likely coming today :)

@onbjerg
Copy link
Contributor

onbjerg commented Jan 22, 2017

@timneutkens 😨

@iamstarkov
Copy link
Contributor

@onbjerg
Copy link
Contributor

onbjerg commented Mar 17, 2017

https://github.com/onbjerg/micro-visualize is still an option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants