Add a warning if some configuration change has wider impact and could cause error?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Add a warning if some configuration change has wider impact and could cause error?

Bartosz Baranowski
Hey guys.


In some cases there are/will be conf changes that might require user to make additional change, for instance [1].
In this case, if user does not edit the name in two places, when remote client tries to reach out, it will fail.

As far as I remember, there as of now, there is no formal constraint feature for config values.
I've talked with Brian about this and it seems and viable option would be to simply add a header to response.
In CLI, it would be clearly visible, in console, it would require a bit  of hacking to present
information/hint what needs to be done to configuration to make it work properly.


Any thoughts?

[1] https://issues.jboss.org/browse/WFCORE-1987
_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Add a warning if some configuration change has wider impact and could cause error?

Brian Stansberry
Any opinions? This is basically a proposal to provide a formal way of including warning data in operations responses.

If we did it it would probably be something like:

{
  “outcome”=>”success”,
  “result”=>”foo”,
  “response-headers”=> {
    “warnings”=[
        “blah blah blah”,
       { “some_complex_issue: details”>[“x”,”y”,’z”]}
    ]
  }
}

So a standard response header whose value is a list of ModelNode, different messages using different node structures, but the norm being a simple string.

Probably OperationContext would need to add a method addResponseWarning to take the details of constructing the header out of the hands of the handler authors.

We’d have to make sure these headers propogate properly through the domain, so the effort is more than simply getting standalone.

If the operation is a composite, there are no per-step response headers so the warning should not assume that the context of the warning is obvious. Warning authors forgetting that point is a possible source of UX issues.

Is this worth the effort? This is driven by this one WFCORE-1987 issue, which is a bit unusual and in an ideal world would go away. But here’s the general pattern involved:

2 attributes in separate parts of the model should be consistent or you wil get bad runtime behavior
User can use a CLI batch to set them both atomically, but web console users can’t do that, so even if the user will do the right thing there will be a moment where they are inconsistent
So, failing is not good (forces users to use CLI + batch)
For the same reason a WARN in the log is not great, as unrelated users may be required to react to log warns, and will be annoyed by seeing them just because their colleague didn’t use a batch

Is that last statement enough reason to add this functionality vs just using the server log to warn, or just not warning at all?

In theory we could use the same facility to report management API deprecation warnings back. Doing that might be annoying though. Those are a less serious problem than this WFCORE-1987 thing.


> On Jan 3, 2017, at 1:06 AM, Bartosz Baranowski <[hidden email]> wrote:
>
> Hey guys.
>
>
> In some cases there are/will be conf changes that might require user to make additional change, for instance [1].
> In this case, if user does not edit the name in two places, when remote client tries to reach out, it will fail.
>
> As far as I remember, there as of now, there is no formal constraint feature for config values.
> I've talked with Brian about this and it seems and viable option would be to simply add a header to response.
> In CLI, it would be clearly visible, in console, it would require a bit  of hacking to present
> information/hint what needs to be done to configuration to make it work properly.
>
>
> Any thoughts?
>
> [1] https://issues.jboss.org/browse/WFCORE-1987
> _______________________________________________
> wildfly-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/wildfly-dev

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Add a warning if some configuration change has wider impact and could cause error?

Jeff Mesnil

> On 6 Jan 2017, at 17:24, Brian Stansberry <[hidden email]> wrote:
>
> Any opinions? This is basically a proposal to provide a formal way of including warning data in operations responses.
>
> If we did it it would probably be something like:
>
> {
>  “outcome”=>”success”,
>  “result”=>”foo”,
>  “response-headers”=> {
>    “warnings”=[
>        “blah blah blah”,
>       { “some_complex_issue: details”>[“x”,”y”,’z”]}
>    ]
>  }
> }

We had a similar discussion while implementing :migrate operations for legacy subsystems and ended up using a "notification-warnings” parameter in the result node (as opposed to the response-headers node).

>
> Is that last statement enough reason to add this functionality vs just using the server log to warn, or just not warning at all?

It’s nice to have for sure and would help having a consistent reporting systems in our management clients (we can imagine having flash warnings in the web console and special text in the CLI written to syserr for example).

In an ideal world, I think that any WARN logged in the server console that is caused by a management operation should be reported to the invoker.
I’d go a step further to constrain the warnings model as a dictionary where each key is a registered logger ID (e.g. XNIO001007) and a value a i18ned string (or a more complex i18ned structure).

Something like:

{
 “outcome”=>”success”,
 “result”=>”foo”,
 “response-headers”=> {
   “warnings” => {
       “XNIO00107” => “some thing happens”,
       “WFCORE1234” => { “some_complex_issue” => [“x”,”y”,’z”]}
   }
 }
}

That’d be very helpful to build our knowledge base and report issues.

> In theory we could use the same facility to report management API deprecation warnings back. Doing that might be annoying though. Those are a less serious problem than this WFCORE-1987 thing.

If you go that way, you could somehow configure the level of reported warnings and let the admin configure the level of reported warnings:

addResponseWarning(String id, int level, String)
addResponseWarning(String id, int level, ModelNode data)

We now have a nice new core-management subsystem where the admin could change the reported warning level (defaulting to something corresponding to serious warnings, as opposed to minor warnings such as API deprecation).

jeff

--
Jeff Mesnil
JBoss, a division of Red Hat
http://jmesnil.net/


_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Add a warning if some configuration change has wider impact and could cause error?

Brian Stansberry

> On Jan 6, 2017, at 10:48 AM, Jeff Mesnil <[hidden email]> wrote:
>
>>
>> On 6 Jan 2017, at 17:24, Brian Stansberry <[hidden email]> wrote:
>>
>> Any opinions? This is basically a proposal to provide a formal way of including warning data in operations responses.
>>
>> If we did it it would probably be something like:
>>
>> {
>> “outcome”=>”success”,
>> “result”=>”foo”,
>> “response-headers”=> {
>>   “warnings”=[
>>       “blah blah blah”,
>>      { “some_complex_issue: details”>[“x”,”y”,’z”]}
>>   ]
>> }
>> }
>
> We had a similar discussion while implementing :migrate operations for legacy subsystems and ended up using a "notification-warnings” parameter in the result node (as opposed to the response-headers node).
>

Yes. I think that was the right approach there, as those warnings were a pretty fundamental part of the result of the operation.

>>
>> Is that last statement enough reason to add this functionality vs just using the server log to warn, or just not warning at all?
>
> It’s nice to have for sure and would help having a consistent reporting systems in our management clients (we can imagine having flash warnings in the web console and special text in the CLI written to syserr for example).
>
> In an ideal world, I think that any WARN logged in the server console that is caused by a management operation should be reported to the invoker.

I was going to say that the private API usage warnings we log couldn’t fit nicely, as they are done via MSC threads that don’t know about any management op. But thinking a bit more I think that’s probably solvable.

> I’d go a step further to constrain the warnings model as a dictionary where each key is a registered logger ID (e.g. XNIO001007) and a value a i18ned string (or a more complex i18ned structure).
>
> Something like:
>
> {
> “outcome”=>”success”,
> “result”=>”foo”,
> “response-headers”=> {
>   “warnings” => {
>       “XNIO00107” => “some thing happens”,
>       “WFCORE1234” => { “some_complex_issue” => [“x”,”y”,’z”]}
>   }
> }
> }
>
> That’d be very helpful to build our knowledge base and report issues.
>

It would be interesting if there were a nice way to integrate such keys with our existing i18n message infrastructure. Really we’d have to do that, or else have the keys unambiguously different from the message ids.

>> In theory we could use the same facility to report management API deprecation warnings back. Doing that might be annoying though. Those are a less serious problem than this WFCORE-1987 thing.
>
> If you go that way, you could somehow configure the level of reported warnings and let the admin configure the level of reported warnings:
>
> addResponseWarning(String id, int level, String)
> addResponseWarning(String id, int level, ModelNode data)
>
> We now have a nice new core-management subsystem where the admin could change the reported warning level (defaulting to something corresponding to serious warnings, as opposed to minor warnings such as API deprecation).
>

It could be controllable via a request-header as well, allowing clients to declare what they want, overriding any server-side settings..

> jeff
>
> --
> Jeff Mesnil
> JBoss, a division of Red Hat
> http://jmesnil.net/

--
Brian Stansberry
Manager, Senior Principal Software Engineer
JBoss by Red Hat




_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Add a warning if some configuration change has wider impact and could cause error?

Bartosz Baranowski
In reply to this post by Brian Stansberry
Thanks Brian.

----- Original Message -----

> From: "Brian Stansberry" <[hidden email]>
> To: "Bartosz Baranowski" <[hidden email]>
> Cc: [hidden email]
> Sent: Friday, January 6, 2017 5:24:42 PM
> Subject: Re: [wildfly-dev] Add a warning if some configuration change has wider impact and could cause error?
>
> Any opinions? This is basically a proposal to provide a formal way of
> including warning data in operations responses.
>
> If we did it it would probably be something like:
>
> {
>   “outcome”=>”success”,
>   “result”=>”foo”,
>   “response-headers”=> {
>     “warnings”=[
>         “blah blah blah”,
>        { “some_complex_issue: details”>[“x”,”y”,’z”]}
>     ]
>   }
> }
>
> So a standard response header whose value is a list of ModelNode, different
> messages using different node structures, but the norm being a simple
> string.
>
> Probably OperationContext would need to add a method addResponseWarning to
> take the details of constructing the header out of the hands of the handler
> authors.
>
> We’d have to make sure these headers propogate properly through the domain,
> so the effort is more than simply getting standalone.

Most likely. I have very little experience with mgmt ops code in domain mode.

>
> If the operation is a composite, there are no per-step response headers so
> the warning should not assume that the context of the warning is obvious.
> Warning authors forgetting that point is a possible source of UX issues.

In such case I think either warning has to be very explicit(detailed what/where/why) or it would become
complex type:
 {
   “outcome”=>”success”,
   “result”=>”foo”,
   “response-headers”=> {
     “warnings”=> {
        warning => {
           operation => { address,name,parameters}
           message = [""]
           },
        warning => {
           operation => { address,name,parameters}
           message = [""]
           }
     }
   }
 }

or

{
   “outcome”=>”success”,
   “result”=>”foo”,
   “response-headers”=> {
     “warnings”=> {
        warning => {
           step => ["1"]
           message = [""]
           },
        warning => {
           step => ["2"]
           message = [""]
           }
     }
   }
 }


Though second option might be too cryptic for console users, unless it is well supported.

>
> Is this worth the effort? This is driven by this one WFCORE-1987 issue, which
> is a bit unusual and in an ideal world would go away. But here’s the general
> pattern involved:
>
> 2 attributes in separate parts of the model should be consistent or you wil
> get bad runtime behavior
> User can use a CLI batch to set them both atomically, but web console users
> can’t do that, so even if the user will do the right thing there will be a
> moment where they are inconsistent
> So, failing is not good (forces users to use CLI + batch)
> For the same reason a WARN in the log is not great, as unrelated users may be
> required to react to log warns, and will be annoyed by seeing them just
> because their colleague didn’t use a batch
>
> Is that last statement enough reason to add this functionality vs just using
> the server log to warn, or just not warning at all?

No warning at all is a bad thing. Unless user is very experienced, such UX will be
perceived as a bad design - well, unless exception/failure will be descriptive enough.
Imagine that you change one tiny detail and server fails to boot up some, seemingly
unrelated service.

So as is, it seem to be a question of fixing current failure message( which for instance
, in case of WFCORE-1987, does not seem viable) vs standardized warning mechanism.

>
> In theory we could use the same facility to report management API deprecation
> warnings back. Doing that might be annoying though. Those are a less serious
> problem than this WFCORE-1987 thing.
>
>
> > On Jan 3, 2017, at 1:06 AM, Bartosz Baranowski <[hidden email]> wrote:
> >
> > Hey guys.
> >
> >
> > In some cases there are/will be conf changes that might require user to
> > make additional change, for instance [1].
> > In this case, if user does not edit the name in two places, when remote
> > client tries to reach out, it will fail.
> >
> > As far as I remember, there as of now, there is no formal constraint
> > feature for config values.
> > I've talked with Brian about this and it seems and viable option would be
> > to simply add a header to response.
> > In CLI, it would be clearly visible, in console, it would require a bit  of
> > hacking to present
> > information/hint what needs to be done to configuration to make it work
> > properly.
> >
> >
> > Any thoughts?
> >
> > [1] https://issues.jboss.org/browse/WFCORE-1987
> > _______________________________________________
> > wildfly-dev mailing list
> > [hidden email]
> > https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
> --
> Brian Stansberry
> Manager, Senior Principal Software Engineer
> JBoss by Red Hat
>
>
>
>

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Add a warning if some configuration change has wider impact and could cause error?

Bartosz Baranowski
In reply to this post by Jeff Mesnil


----- Original Message -----

> From: "Jeff Mesnil" <[hidden email]>
> To: "Brian Stansberry" <[hidden email]>
> Cc: "Bartosz Baranowski" <[hidden email]>, [hidden email]
> Sent: Friday, January 6, 2017 5:48:11 PM
> Subject: Re: [wildfly-dev] Add a warning if some configuration change has wider impact and could cause error?
>
>
> > On 6 Jan 2017, at 17:24, Brian Stansberry <[hidden email]>
> > wrote:
> >
> > Any opinions? This is basically a proposal to provide a formal way of
> > including warning data in operations responses.
> >
> > If we did it it would probably be something like:
> >
> > {
> >  “outcome”=>”success”,
> >  “result”=>”foo”,
> >  “response-headers”=> {
> >    “warnings”=[
> >        “blah blah blah”,
> >       { “some_complex_issue: details”>[“x”,”y”,’z”]}
> >    ]
> >  }
> > }
>
> We had a similar discussion while implementing :migrate operations for legacy
> subsystems and ended up using a "notification-warnings” parameter in the
> result node (as opposed to the response-headers node).
>
> >
> > Is that last statement enough reason to add this functionality vs just
> > using the server log to warn, or just not warning at all?
>
> It’s nice to have for sure and would help having a consistent reporting
> systems in our management clients (we can imagine having flash warnings in
> the web console and special text in the CLI written to syserr for example).
>
> In an ideal world, I think that any WARN logged in the server console that is
> caused by a management operation should be reported to the invoker.

Yes, in a way. But difference here is that WARN could come from any place, so we would have to intercept logger.
Rather than rely on dev to be through and be aware of implications of mgmt ops. Just guessing that it might become quite complicated.
For initial implementation I would just stick to original problem.

> I’d go a step further to constrain the warnings model as a dictionary where
> each key is a registered logger ID (e.g. XNIO001007) and a value a i18ned
> string (or a more complex i18ned structure).
>
> Something like:
>
> {
>  “outcome”=>”success”,
>  “result”=>”foo”,
>  “response-headers”=> {
>    “warnings” => {
>        “XNIO00107” => “some thing happens”,
>        “WFCORE1234” => { “some_complex_issue” => [“x”,”y”,’z”]}
>    }
>  }
> }
>
> That’d be very helpful to build our knowledge base and report issues.
>
> > In theory we could use the same facility to report management API
> > deprecation warnings back. Doing that might be annoying though. Those are
> > a less serious problem than this WFCORE-1987 thing.
>
> If you go that way, you could somehow configure the level of reported
> warnings and let the admin configure the level of reported warnings:
>
> addResponseWarning(String id, int level, String)
> addResponseWarning(String id, int level, ModelNode data)

Most likely level could be equal to logging.Level.

>
> We now have a nice new core-management subsystem where the admin could change
> the reported warning level (defaulting to something corresponding to serious
> warnings, as opposed to minor warnings such as API deprecation).

Where? Or are you talking about above example?

>
> jeff
>
> --
> Jeff Mesnil
> JBoss, a division of Red Hat
> http://jmesnil.net/
>
>

_______________________________________________
wildfly-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/wildfly-dev
Loading...