search

Glimpse / Home

Project Glimpse: Node Edition - Spend less time debugging and more time developing.
http://node.getglimpse.com
Other
252 stars 9 forks source link

[Logs] console.trace calls show up as errors #37

Closed lostintangent closed 7 years ago

lostintangent commented 8 years ago

This is a pretty minor issue, but since error messages display the red icon, it would be great if users could visually scan their logs for issues, without seeing any false negatives.

Repro steps:

  1. Add a call to console.trace into your app
  2. Run the app to exercise the code that calls #1
  3. Open up the Glimpse client and select the respective request in the list
  4. Select the Logs tab

Expected: To see the call to console.trace, displayed as an informational message Actual: The trace call shows up, but it is labeled as an error.

capture

nikmd23 commented 8 years ago

@lostintangent, I'm a bit confused as to what the issue you're reporting is.

Is it that this message isn't actually an "Error" but it being reported as such?

lostintangent commented 8 years ago

That screenshot is showing the result of me calling console.trace within an app. The message is the stack that was generated as a result, but Glimpse seems to be categorizing the log as an error, which it isn't. Apologies for the ambiguity, I just added repro steps to the bug to hopefully make this clearer.

nikmd23 commented 8 years ago

Oh! I see, okay. Thanks for the clarification.

mike-kaufman commented 8 years ago

See https://github.com/nodejs/node/blob/master/lib/console.js#L82_L90. Console.trace(...) writes output to through Console.error(...). IIRC, we're not proxying trace.

nikmd23 commented 8 years ago

Okay, it looks like the issue here is a difference in the implementation of the/a spec.

The DevTools Working Group says that trace is "equivalent to console.log except that it also adds stack trace from the point where the method was called". This is in line with @lostintangent's expectation. However, Node's documentation states that calls to .trace should be written to stderr. (That's true for both 4.5 LTS and 6.6.)

I do find Node's implementation to be a bit funny, but we should probably stick to their definition until we have a bit more user feedback.

That said, calls to console.trace() in the browser should have the behavior that @lostintangent described. We need to test to make sure this is the case. @newtonjain, please make sure this case is covered in your related work.

mike-kaufman commented 8 years ago

We can fairly easily proxy trace for node such that it still writes to stderr, but the glimpse message has an info or a warning. This is a small, low-risk change.

nikmd23 commented 8 years ago

I like @mike-kaufman's suggestion, and think that we should make sure the Glimpse message has an Info Debug tag on it.

nikmd23 commented 8 years ago

console.trace() calls from Node are still showing up as Errors. They should be marked as Debug.

mike-kaufman commented 8 years ago

Addressed with PR https://github.com/Glimpse/Glimpse.Node/pull/633.

nikmd23 commented 8 years ago

👌

I did find a related issue while testing this (https://github.com/Glimpse/Glimpse.Browser.Agent/issues/86), but this looks good.

nikmd23 commented 7 years ago

This is now available in Glimpse for Node 0.15.2. Please see our announcement issue for more information.