Concerns around Lightblue's Error

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Concerns around Lightblue's Error

dcrissman
I have some concerns around the Error class that I would like to discuss and maybe see if we can make some improvements.

* I am not sure it is useful or a good idea to send the stacktrace to the client.
* I don't see that this stacktrace was ever printed in the server logs, which would potentially be useful depending on what the error is.
* Having the stacktrace concatenated makes it hard to read.
* Putting all exception into the Error container prevents catching specific exceptions

As an aside, and this one isn't a huge deal, but java.lang.Error already exists in the core framework and using the same name could be confusing.

Example error json:
{"objectType":"error","context":"find(department:1.0.0)","errorCode":"crud","msg":"java.lang.IllegalArgumentException: Empty identity fields\n\tat com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:48)\n\tat com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:61)\n\tat com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:68)\n\tat com.redhat.lightblue.mediator.QueryPlanNodeExecutor.<init>(QueryPlanNodeExecutor.java:86)\n\tat com.redhat.lightblue.mediator.CompositeFindImpl.init(CompositeFindImpl.java:102)\n\tat com.redhat.lightblue.mediator.CompositeFindImpl.find(CompositeFindImpl.java:198)\n\tat com.redhat.lightblue.mediator.Mediator.find(Mediator.java:350)\n\tat com.redhat.lightblue.crud.ldap.ITCaseLdapCRUDControllerTest.series2_phase2_Department_FindWithRoles(ITCaseLdapCRUDControllerTest.java:207)\n\tat sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n\tat sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)\n\tat sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n\tat java.lang.reflect.Method.invoke(Method.java:606)\n\tat org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)\n\tat org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)\n\tat org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)\n\tat org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)\n\tat org.junit.rules.Verifier$1.evaluate(Verifier.java:35)\n\tat org.junit.rules.RunRules.evaluate(RunRules.java:20)\n\tat org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)\n\tat org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)\n\tat org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)\n\tat org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)\n\tat org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)\n\tat org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)\n\tat org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)\n\tat org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)\n\tat org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)\n\tat org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)\n\tat org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)\n\tat org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)\n\tat org.junit.rules.RunRules.evaluate(RunRules.java:20)\n\tat org.junit.runners.ParentRunner.run(ParentRunner.java:309)\n\tat org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)\n\tat org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)\n\tat org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)\n\tat org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:675)\n\tat org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)\n\tat org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)\n"}
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

bserdar
This is a wrong use of the Error class. You should not put
exception.toString into an Error. Instead, you should throw a
meaningful error with a meaningful error code and some context
information. Error.push and Error.pop gives a better stack-trace like
context information, so you should use those on entry/exit of some of
the top-level methods.

On Fri, Jan 16, 2015 at 1:50 PM, dcrissman [via lightblue-dev]
<[hidden email]> wrote:

> I have some concerns around the Error class that I would like to discuss and
> maybe see if we can make some improvements.
>
> * I am not sure it is useful or a good idea to send the stacktrace to the
> client.
> * I don't see that this stacktrace was ever printed in the server logs,
> which would potentially be useful depending on what the error is.
> * Having the stacktrace concatenated makes it hard to read.
> * Putting all exception into the Error container prevents catching specific
> exceptions
>
> As an aside, and this one isn't a huge deal, but java.lang.Error already
> exists in the core framework and using the same name could be confusing.
>
> Example error json:
> {"objectType":"error","context":"find(department:1.0.0)","errorCode":"crud","msg":"java.lang.IllegalArgumentException:
> Empty identity fields\n\tat
> com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:48)\n\tat
> com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:61)\n\tat
> com.redhat.lightblue.metadata.DocIdExtractor.<init>(DocIdExtractor.java:68)\n\tat
> com.redhat.lightblue.mediator.QueryPlanNodeExecutor.<init>(QueryPlanNodeExecutor.java:86)\n\tat
> com.redhat.lightblue.mediator.CompositeFindImpl.init(CompositeFindImpl.java:102)\n\tat
> com.redhat.lightblue.mediator.CompositeFindImpl.find(CompositeFindImpl.java:198)\n\tat
> com.redhat.lightblue.mediator.Mediator.find(Mediator.java:350)\n\tat
> com.redhat.lightblue.crud.ldap.ITCaseLdapCRUDControllerTest.series2_phase2_Department_FindWithRoles(ITCaseLdapCRUDControllerTest.java:207)\n\tat
> sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n\tat
> sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)\n\tat
> sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n\tat
> java.lang.reflect.Method.invoke(Method.java:606)\n\tat
> org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)\n\tat
> org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)\n\tat
> org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)\n\tat
> org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)\n\tat
> org.junit.rules.Verifier$1.evaluate(Verifier.java:35)\n\tat
> org.junit.rules.RunRules.evaluate(RunRules.java:20)\n\tat
> org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)\n\tat
> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)\n\tat
> org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)\n\tat
> org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)\n\tat
> org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)\n\tat
> org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)\n\tat
> org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)\n\tat
> org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)\n\tat
> org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)\n\tat
> org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)\n\tat
> org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)\n\tat
> org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)\n\tat
> org.junit.rules.RunRules.evaluate(RunRules.java:20)\n\tat
> org.junit.runners.ParentRunner.run(ParentRunner.java:309)\n\tat
> org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)\n\tat
> org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)\n\tat
> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)\n\tat
> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:675)\n\tat
> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)\n\tat
> org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)\n"}
>
>
> ________________________________
> If you reply to this email, your message will be added to the discussion
> below:
> http://dev.forum.lightblue.io/Concerns-around-Lightblue-s-Error-tp294.html
> To start a new topic under lightblue-dev, email
> [hidden email]
> To unsubscribe from lightblue-dev, click here.
> NAML
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

dcrissman
I didn't call toString() on this exception. If you look at some of the Error#get() methods you will see there that it prints the stacktrace as the message.
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

bserdar
Ideally, all methods that are significant in terms of  error context
should be of the form:

void method() {
   Error.push("method");
   try {
    ...
    } finally {
      Error.pop();
   }
}

And whenever there is an exception, methods should throw Error.get(...).

Stacktraces in Error is mainly used as a catchall, to handle any
exceptions that are not handled as above. If you see stack traces in
Error somewhere else, that needs to be fixed. The above construct
returns  an error code, some context information related to that error
code, and all the relevant context annotated with additional
information, and it is much more useful than stack traces. For
instance, all rest calls start with pushing the operation, entity name
and entity version, so any error thrown will have those information
regardless from where it is thrown.



On Fri, Jan 16, 2015 at 2:05 PM, dcrissman [via lightblue-dev]
<[hidden email]> wrote:

> I didn't call toString() on this exception. If you look at some of the
> Error#get() methods you will see there that it prints the stacktrace as the
> message.
>
> ________________________________
> If you reply to this email, your message will be added to the discussion
> below:
> http://dev.forum.lightblue.io/Concerns-around-Lightblue-s-Error-tp294p296.html
> To start a new topic under lightblue-dev, email
> [hidden email]
> To unsubscribe from lightblue-dev, click here.
> NAML
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

dcrissman
I found context information very useful when debugging json files that I created for testing, it pinpointed exactly where in the document had something invalid. So I don't take issue with the concept of a context. That said, I am still unclear on the distinction between the context and exceptions. I found the description below in the user guide, but it still does not define what exactly the context is other than "context of execution" which I don't understand what that means and "similar to a strack trace", in which case I have to ask why do we need both?
context: A string delimited with "/" characters that define the context of execution where the error occurred, similar to a stack trace.
To my question about the stacktrace being returned to the client, could we instead return the message of the exception to the client and log the full stack trace to the server log? Maybe if there is an unknown errorCode, we use the Class name of the exception?
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

bserdar
Stacktrace doesn't have context variables in it, only the call stack.
We push data to the error context as well, so we know certain values
as well as the relevant call stack. For instance, the error context
for an insert call looks like:

/insert/user/1.0.0/ParseMetadata/...etc

We don't need both the stack trace and error context. We should not
return the stack trace in the error message.

For catch-all cases: I agree we should log the stack trace, and return
the exception in the message.

On Mon, Jan 19, 2015 at 8:36 AM, dcrissman [via lightblue-dev]
<[hidden email]> wrote:

> I found context information very useful when debugging json files that I
> created for testing, it pinpointed exactly where in the document had
> something invalid. So I don't take issue with the concept of a context. That
> said, I am still unclear on the distinction between the context and
> exceptions. I found the description below in the user guide, but it still
> does not define what exactly the context is other than "context of
> execution" which I don't understand what that means and "similar to a strack
> trace", in which case I have to ask why do we need both?
>
> context: A string delimited with "/" characters that define the context of
> execution where the error occurred, similar to a stack trace.
>
> To my question about the stacktrace being returned to the client, could we
> instead return the message of the exception to the client and log the full
> stack trace to the server log? Maybe if there is an unknown errorCode, we
> use the Class name of the exception?
>
> ________________________________
> If you reply to this email, your message will be added to the discussion
> below:
> http://dev.forum.lightblue.io/Concerns-around-Lightblue-s-Error-tp294p298.html
> To start a new topic under lightblue-dev, email
> [hidden email]
> To unsubscribe from lightblue-dev, click here.
> NAML
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

dcrissman
Change made to Error class to return the message rather than the stacktrace.
See: https://github.com/lightblue-platform/lightblue-core/pull/295
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

dcrissman
In reply to this post by bserdar
Allow me to change the topic somewhat.

I see places in the Lightblue framework that use pure Exceptions (eg. IllegalArgumentException, UnsupportedOperationException, etc.) and others that use Lightblue's Error subsystem. When is it appropriate to use one over the other?
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

bserdar
Ideally, only Error should be used.

On Mon, Jan 19, 2015 at 1:50 PM, dcrissman [via lightblue-dev]
<[hidden email]> wrote:

> Allow me to change the topic somewhat.
>
> I see places in the Lightblue framework that use pure Exceptions (eg.
> IllegalArgumentException, UnsupportedOperationException, etc.) and others
> that use Lightblue's Error subsystem. When is it appropriate to use one over
> the other?
>
>
> ________________________________
> If you reply to this email, your message will be added to the discussion
> below:
> http://dev.forum.lightblue.io/Concerns-around-Lightblue-s-Error-tp294p301.html
> To start a new topic under lightblue-dev, email
> [hidden email]
> To unsubscribe from lightblue-dev, click here.
> NAML
Reply | Threaded
Open this post in threaded view
|

Re: Concerns around Lightblue's Error

dcrissman
In reply to this post by bserdar
Conversation about context has been moved to: https://github.com/lightblue-platform/lightblue-docs-user/issues/63