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

feat: Auto Instrumentation for Awake #998

Merged
merged 35 commits into from
Oct 28, 2022
Merged

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Oct 4, 2022

The idea is to start and finish spans automatically for every Awake method in all MonoBehaviour within the project.
Noticeable pitfall: Not every MonoBehaviour implements Awake.

The plan:

  • Hook up to the compilation pipeline to get the Assembly-CSharp.dll
  • Add options flag to enable/disable feature and put it on the window
  • Insert Start Span as the very first instruction
  • Insert Finish Span before every return
  • Enable Trace Sample Rate for smoke testing
  • Enable Performance Auto Instrumentation for smoke testing
  • Smoke test looking for a app.start transaction
  • Smoke test looking for awake span

How it works right now:

We look for any Awake in any class within Assembly-CSharp.dll that inherits from MonoBehaviour.
I.e.

public class MyMonoBehaviour : MonoBehaviour
{
    private void Awake()
    {
        Debug.Log("This is an Awake call!");
    }
}

and we modify it to look like

public class MyMonoBehaviour : MonoBehaviour
{
    private void Awake()
   {
       SentrySdk.GetSpan()?.StartChild("awake", $"MyMonoBehaviour on {gameObject.name}");
 
       Debug.Log("This is an Awake call!");
 
       SentrySdk.GetSpan()?.Finish(SpanStatus.Ok);
   }
}

The IL code for the original

.method private hidebysig
    instance void Awake () cil managed
{
    // Method begins at RVA 0x2120
    // Header size: 1
    // Code size: 11 (0xb)
    .maxstack 8
 
    // Debug.Log("This is an Awake call!");
    IL_0000: ldstr "This is an Awake call!"
    IL_0005: call void [UnityEngine.CoreModule]UnityEngine.Debug::Log(object)
    // }
    IL_000a: ret
} // end of method MyMonoBehaviour::Awake

And the modified IL code

.method private hidebysig
 
    instance void Awake () cil managed
{
    // Method begins at RVA 0x22b7
    // Header size: 1
    // Code size: 32 (0x20)
    .maxstack 8
 
    // SentryMonoBehaviour.Instance.StartAwakeSpan(this);
    IL_0000: call class [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour::get_Instance()
    IL_0005: ldarg.0
    IL_0006: callvirt instance void [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour::StartAwakeSpan(class [UnityEngine]UnityEngine.MonoBehaviour)
    // Debug.Log("This is an Awake call!");
    IL_000b: ldstr "This is an Awake call!"
    IL_0010: call void [UnityEngine.CoreModule]UnityEngine.Debug::Log(object)
    // SentryMonoBehaviour.Instance.FinishAwakeSpan();
    IL_0015: call class [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour::get_Instance()
    IL_001a: call instance void [Sentry.Unity]Sentry.Unity.SentryMonoBehaviour::FinishAwakeSpan()
    // }
    IL_001f: ret
} // end of method MyMonoBehaviour::Awake

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 6b08bf1

@bitsandfoxes bitsandfoxes marked this pull request as ready for review October 14, 2022 15:08
Comment on lines 117 to +127
// Skip the session init requests (there may be multiple of them). We can't skip them by a "positive"
// because they're also repeated with standard events (in an envelope).
Debug.Log("SMOKE TEST: Skipping all non-event requests");
Debug.Log("SMOKE TEST: Skipping all session requests");
for (; currentMessage < 10; currentMessage++)
{
if (t.CheckMessage(currentMessage, "'type':'event'"))
if (t.CheckMessage(currentMessage, "'type':'transaction'"))
{
break;
}
}
Debug.Log($"SMOKE TEST: Done skipping non-event requests. Last one was: #{currentMessage}");
Debug.Log($"SMOKE TEST: Done skipping session requests. Last one was: #{currentMessage}");
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels strange. The comments talk about skipping sessions, but really you're skipping everything until you get a transaction. As long a there are less than 10 of them... 🤔

@mattjohnsonpint
Copy link
Contributor

Generally looks good. Commented with some minor nits.

Didn't test it out though - I'd need more context.

@bitsandfoxes bitsandfoxes merged commit d1c0491 into main Oct 28, 2022
@bitsandfoxes bitsandfoxes deleted the feat/awake-instrument branch October 28, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants