BHoM / BHoM_UI

GNU Lesser General Public License v3.0
9 stars 5 forks source link

BHoM_UI: Add log output to the adapter actions #311

Open pawelbaran opened 3 years ago

pawelbaran commented 3 years ago

Description:

Currently the adapter actions have 2 outputs: objects and success. In many cases, the latter is not very informative, e.g. is it success if 99% of objects is processed correctly and one fails? Or if one aspect of object processing fails? On top of that, in many cases the number of warnings and errors thrown by the adapter action exceeds 15, leading to them being effectively hidden from the user.

I believe that a good way to resolve the above could be adding log output that would store the messages recorded during the adapter action in a more user friendly way than the baloon on top of the component - it would also allow logging partial success or formatting of the messages. I am aware that such mechanism could be hard to realize in with the current UI framework, but I believe it would be worth considering at least.

Would be good to hear thoughts of others too @al-fisher @rwemay @IsakNaslundBh @FraserGreenroyd

alelom commented 3 years ago

Thanks @pawelbaran , very useful discussion.

Adding a log output is something I had proposed in the past too. However, since then, I realized most of the logging can be effectively done by leveraging the Reflection.Compute.RecordError/Warning/Note methods. For example, I've always used Warning to expose a partial success to the user, and Note for anything that shouldn't worry beginner users. This is why I was especially keen on solving issues such as https://github.com/BHoM/Grasshopper_Toolkit/issues/480.

We can surely propose an additional log output, but we'd need to clarify its role with respect to the Reflection.Compute.Record* methods. My tentative analysis follows:

  1. So far, the only real limitation of the Reflection.Compute.Record* VS a log output that I can see is the limit of 15 elements.
  2. Adding another log output requires clarification as to per what should end up in the Reflection.Compute.Record* and what should end up in the log, because both are exposed to the User.
  3. The success boolean is mainly useful to chain multiple Actions. Being a boolean it can't represent "shades of grey".

Now my answers to these points:

  1. I honestly haven't ever seen (1) as an actual limitation. If you are throwing more than that number of messages to the user, I think that there's something going wrong in the backend: you can't expect an user to read much more than that.
    Maybe we could consider limiting the number of messages per each category (Error/Warning/Note) rather than the total, but I'm not sure this would be game-changing.
  2. We could solve (2) by saying that everything we record using the Reflection.Compute.Record* should end up in the log, but that would serve only as an "extension" to the message system. A bit weak.
  3. The success boolean can't represent shades of grey, but I think that's OK.
    All we can do is leave the responsibility to the Adapter implementer, which should decide whether the action was a success or not (e.g. even if some of the input was incorrectly processed). "Shades of grey" can only be represented via the messages and/or log.
    I guess that, to leave the chance to give a "I don't know" answer, we could make it a nullable boolean. But I don't think this would be game-changing: if you are unsure, why not default that to false?

So, all in all, I think that maybe we could consider another system, instead of another log output.
Since you are mentioning scenarios where multiple (>15) errors could occur, I've also considered in the past to record in a proper log in a file every time an action is triggered. For example, you could have that file to record more developer-centred information, storing a proper stack trace for exceptions, line number where the error occurred, etc. Since this would be separate from the UI, we should not worry about exposing user-unfriendly information. This could work well also considering https://github.com/BHoM/BHoM_Engine/issues/2011.

pawelbaran commented 3 years ago

Thanks @alelom for the detailed reply, in general I agree with everything you wrote. I let myself answer your points, more as a food for thought than anything else:

  1. I honestly haven't ever seen (1) as an actual limitation. If you are throwing more than that number of messages to the user, I think that there's something going wrong in the backend: you can't expect an user to read much more than that. Maybe we could consider limiting the number of messages per each category (Error/Warning/Note) rather than the total, but I'm not sure this would be game-changing.

Yeah, that is a very valid point that the number of warnings/errors is often too large. However, that is usually caused by the adapters capturing various bits and pieces, which might or might not be relevant to the user - hard to decide what to hide and what to show. On top of that, often the messages are duplicated because each of them contains the identifier of failing object. Something that ofc should be improved in each adapter, but the more help would come from the central repos the better. A log could be a simple (yet crowbar-like) measure to allow post-processing the captured messages. In other words, I would not push the log as an ultimate solution, but some more freedom in shaping the final form of the messages would be nice.

  1. We could solve (2) by saying that everything we record using the Reflection.Compute.Record* should end up in the log, but that would serve only as an "extension" to the message system. A bit weak.

Yeah, I thought about that. Another alternative would be to make parsing of the Reflection.Compute.Record* pipeline a bit more user friendly (a component similar to Explode that would return organized messages from the adapter action?).

  1. The success boolean can't represent shades of grey, but I think that's OK.

I was also thinking about an enum that would allow some shades of grey. This however could quickly lead to a myriad of requests for certain output 🙈

al-fisher commented 3 years ago

@alelom @pawelbaran my comments on this issue here are I think relevant also in the general sense of the adaptor actions. Here considering specifically Pull for buffering and asynchronous behaviour - https://github.com/BHoM/BHoM_UI/issues/236#issuecomment-696785587

adecler commented 3 years ago

Great conversation! I agree with what was said so far. I would add that this conversation should probably be extended to all components. We already have a log system that can be accessed by using the AllEvents component. This is, however, very limited right now since we cannot isolate per component with the current interface.

So we could extend that functionality by allowing components like CurrentEvents and AllEvents to take a component as input to filter by it. The other option is obviously to add an optional log output to all components.

We will have to see how well it works for other UIs like Excel though.

We can obviously have both but, in this case, I am not sure we should ?

adecler commented 3 years ago

Where do we stand on this now that the GetEvents component exists ?