Skip to content
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

[Feature Request] adding a weaver attribute for logging methods #21

Open
loic-lopez opened this issue Oct 27, 2019 · 9 comments
Open

[Feature Request] adding a weaver attribute for logging methods #21

loic-lopez opened this issue Oct 27, 2019 · 9 comments

Comments

@loic-lopez
Copy link

loic-lopez commented Oct 27, 2019

I think it will be good to add a weaver like that :

The attribute

 public class MethodLogger : Attribute
    {
        void OnMethodWillExecute(parameters, method) 
        {
           Debug.Log("entered in method " + method + " with parameters " + parameters);
        }
       
        void OnMethodExecuted(returnValue, method)
       {
           Debug.Log("finished in method " + method + " with return value " + returnValue);
       }
    }

The use case:

using UnityEngine;
using Arch.Internal.Advices;

public class SceneEventManager : MonoBehaviour
{
   [MethodLogger]
   public void Test()
   {
      Debug.Log("Invoked Test");
   }

   private void Update()
   {
       if (Input.anyKeyDown)
       {
                Test();
       }
   }
}
@ByronMayne
Copy link
Owner

Hey Loic,

Yeah this is a feature that makes sense to have. Right now I only have a few sample plugins available just to help people design their own plugins. I am sure this could be something that can be added to the default ones. There are just a few things to take into account.

  1. Multiple return paths.
{
   if(value < 5) return "Fizz";
   else return "Buzz"
}```
In this case we just have to insert instructions before every return. 

2) IEnumerator 
```public IEnumerator<int> Foo() 
{
   for(int i = 0; i < 10; i++) 
   {
      yield return i;
   }
}```
Do we return once per yield? 
3) Function Return Types
```public Action Foo()
{
   return Bar; 
}```
If we just took the action and did a `.ToString()` you get a pretty useless log of `System.Action`. 
4) Handling exceptions
The code that is injected into the user code should never throw exceptions as most people will have no clue how to debug code that is injected into a compiled assembly. So we have to be extra 
5) Exception Logging
If a exception is thrown by the users code should we wrap it in a try catch and return log it as well? 

@loic-lopez
Copy link
Author

@ByronMayne thanks for your response.

1) Multiple return paths.

I'm agree with inserting instructions before every return.

2) IEnumerator

I think we can do something like

[MethodLogger(Yield = true)]
public IEnumerator<int> Foo() 
{
   for(int i = 0; i < 10; i++) 
   {
      yield return i;
   }
}

3) Function Return Types

public Action Foo()
{
   return Bar; 
}

In case of callback returned ignore the behavior of logging?

4) Exception Logging & handling

I think we can propose to the user also an option:

[MethodLogger(ExceptionLogging= true)]
public void Foo() 
{
    throw new System.NotImplementedException();
}

@loic-lopez
Copy link
Author

loic-lopez commented Oct 28, 2019

@ByronMayne also at the time i'm on unity 2019.2 do you have plan to update the library?

I can't add an assembly to the weaver settings

@ghost
Copy link

ghost commented Jan 28, 2020

@ByronMayne I think the generated IL code always results in one ret instruction. When multiple return paths are found, it just branches to the return after popping (if needed) from the stack.

Furthermore, IEnumerators are kind of a weird thing. You can either yield return, where the generated IL code produces a nested class in which you want to inject the code or a normal return, whereas it should be treated as normal function.

I'm posting this here to say that I've created a Decorator component and attribute which you can override and pretty much does what this issue is about - and some more. It's a bit messy so I don't know if I'll get time to wrap it and send it here. My only issue is that I always handle IEnumerator functions as normal functions, ignoring if there's a yield in there. I do not know if I'll find time to fix that.

On a further note, I fixed the problem where you couldn't add assemblies but I had to change some stuff. I didn't implement it to work differently for older Unity versions. All I did was ALWAYS apply the modified properties (but not run the AssemblyUtility.DirtyAllScripts) and made a cache to be able to revert the modified properties if you want to (if you haven't called AssemblyUtility.DirtyAllScripts).

@ByronMayne
Copy link
Owner

Hey Salokinsomisa,

Even though I have written this tool that write and manipulates IL I am still not very good with it. There are so many case I just don't know of so I get a bit wary. I am trying to come up with a more elegant way to write unit tests to make sure I also don't break anything.

I am currently in the works of a refactor for Weaver because of some of the old design decisions I took as well as this branch being unable to support newer versions of Unity. In this I am also rewriting all the default plugins to make the code much cleaner and fix a few small bugs. If you want to post your code I could use it as a reference for what you were doing.

One feature I am adding which I am really excited for is the recursive flag for methods. For example [MethodLog(Recursive=true)] would log this method and every method it calls into recursively. It would only inject logs into methods that belong in assemblies in the weaver list.

@ghost
Copy link

ghost commented Feb 1, 2020

Hello! The fix for the assemblies is scattered throughout the source code ( I removed many unnecessary calls to AssemblyUtility.DirtyAllScripts because if you had many assemblies it could take even up to 7 seconds (for me) to load them. Furthermore I populate the assemblies only once when the WeaverSettings editor is enabled for the first time. I did that because when hitting play a domain reload happens and adding 7 seconds to the time it takes to load the game EACH time you hit play is massive.

Apart from that, the DecoratorComponent that does what this issue is all about is seperated into two files.

What parts of the code do you want? Both the fixes and the Decorator or one of the two?

@lorenblue
Copy link

Hey guys, I know this thread is a few years old but I am very interested in such an attribute (I just want to log the type on which the method was called)! I'm on Unity 2021.

@lorenblue
Copy link

Sorry to bump again, but I am still super interested!

@ByronMayne
Copy link
Owner

I am no longer working on this repository as I can't due to NDA. If you would like toe add the feature I can accept PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants