Skip to content

BHoM_UI: Add log output to the adapter actions #311

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

Open
pawelbaran opened this issue Sep 24, 2020 · 5 comments
Open

BHoM_UI: Add log output to the adapter actions #311

pawelbaran opened this issue Sep 24, 2020 · 5 comments
Assignees
Labels
type:feature New capability or enhancement type:question Ask for further details or start conversation

Comments

@pawelbaran
Copy link
Member

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

@pawelbaran pawelbaran added type:feature New capability or enhancement type:question Ask for further details or start conversation labels Sep 24, 2020
@alelom
Copy link
Member

alelom commented Sep 24, 2020

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 BHoM/Grasshopper_UI#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 BHoM/BHoM_Engine#2011.

@pawelbaran
Copy link
Member Author

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
Copy link
Member

@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 -
#236 (comment)

@adecler
Copy link
Member

adecler commented Sep 25, 2020

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.

  • The optional output solution is the simplest one (although optional input/outputs are not in Excel yet) but there some annoying sides to it. For example, you might not necessarily want to always have to allocated cells just for potential log outputs.
  • The external component option is easy for the user (just select the cell you want the events for) but might be a bit more technically challenging due to how Excel works. It is a great option for situational debugging though as it isn't in the way an only there when you need it (i.e. when the error messages are not enough).

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

@adecler
Copy link
Member

adecler commented Nov 12, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement type:question Ask for further details or start conversation
Projects
None yet
Development

No branches or pull requests

4 participants