Skip to content

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Sep 12, 2024

Description

This PR adds play-mode editor analytics for tracking input action code-setup paths where users are using code/APIs to setup actions instead of assets.

Changes made

Added basic per-function analytics that also get aggregated into a simple boolean expression whether code setup paths are being used or not.

I personally think this is quite brittle and may generate false positives since it require manual suppression to avoid it. I see no current way around it. Notably in OnScreenStick and in TrackedPoseDriver. All other (current) references to this API surface seems to be from test cases where the analytics are anyway stubbed away.

Testing

Added test case to track analytics being recorded similar to existing analytics unit tests.

Tested with Analytics SDK locally. Also will be confirmed with analytics team in backend before merged.

Risk

Small, main risk is recording false positives indirectly through internal calls (in the future). Might be that this is heavily used on XR side? If it is it might require suppression there.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Analytics_ShouldReportCodeAuthoringAnalytic added Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@ekcoh ekcoh changed the title ISX-2099 Code-setup analytics NEW: Code-setup analytics Sep 12, 2024
@ekcoh ekcoh changed the title NEW: Code-setup analytics NEW: ISX-2099 Code-setup analytics Sep 12, 2024
@ekcoh ekcoh changed the title NEW: ISX-2099 Code-setup analytics NEW: ISX-2099 InputAction code-setup (authoring) analytics Sep 12, 2024
@ekcoh ekcoh marked this pull request as ready for review September 12, 2024 13:16
@ekcoh ekcoh requested a review from ritamerkl September 12, 2024 13:40
Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Looks good to me.


// NOTE: We do not want to trigger entering/exiting play-mode for this small data-sanity check
// so just stick to triggering it explicitly. A better test would have been an editor test
// going in and out of play-mode for real but not clear if this is really possible.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this is possible - there are other tests that do this E.g. texture streaming tests in trunk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would love to learn how to do that properly. I tried it but when I programmatically exited play-mode the test aborted. We state its possible in the docs, but couldn't see any reference to how: https://docs.unity3d.com/Packages/com.unity.test-framework@1.1/manual/edit-mode-vs-play-mode-tests.html

Note: You can also control entering and exiting Play Mode from your Edit Mode test. This allow your test to make changes before entering Play Mode.

EditorApplication.playMode doesn't seem to do the job for me....

Copy link
Collaborator

Choose a reason for hiding this comment

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

In and edit and play mode test You should be able to use code like this:

yield return new EnterPlayMode();
yield return new ExitPlayMode();

See this for more details internally:
https://github.cds.internal.unity3d.com/unity/com.unity.memoryprofiler/blob/46687aa966bea49d093041968fb6d40cc8fd39f4/com.unity.memoryprofiler/Tests/Editor/SceneRootsAndAssetBundles/ReferenceToTests.cs#L58

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will land this as-is and file a separate task on this. if ok @lyndon-unity ? There are many places to change to that / correct triggers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A ticket have now been filed.

@ekcoh ekcoh requested a review from Pauliusd01 September 13, 2024 09:21
Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

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

Nice, only minor comments.

@Pauliusd01
Copy link
Collaborator

Will check this today/tomorrow morning

@ekcoh ekcoh merged commit 026cdcc into develop Sep 27, 2024
77 checks passed
@ekcoh ekcoh deleted the isx-2099-code-setup-analytic branch September 27, 2024 10:05
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.

4 participants