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: iOS native bridge for scope sync #296

Merged
merged 23 commits into from
Sep 18, 2021
Merged

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Aug 26, 2021

Sync between .NET and iOS native via the scope observer interface and a native bridge.
Things that get synched:

  • Breadcrumbs
  • Extras
  • Tags
  • User

Remaining open: additional data for user (tracked by #314)

package-dev/Runtime/SentryInitialization.cs Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/NativeBridge.m Outdated Show resolved Hide resolved
src/Sentry.Unity/UnityNativeScopeObserver.cs Outdated Show resolved Hide resolved
src/Sentry.Unity/UnityNativeScopeObserver.cs Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/NativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
src/Sentry.Unity/UnityNativeScopeObserver.cs Outdated Show resolved Hide resolved
src/Sentry.Unity/UnityNativeScopeObserver.cs Outdated Show resolved Hide resolved
src/Sentry.Unity/UnityNativeScopeObserver.cs Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

A quick first pass.

samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
samples/unity-of-bugs/Assets/Scripts/SentryNativeBridge.m Outdated Show resolved Hide resolved
@bitsandfoxes bitsandfoxes marked this pull request as ready for review September 16, 2021 12:55
@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2021

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- iOS native bridge for scope sync ([#296](https://github.com/getsentry/sentry-unity/pull/296))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 32657e8

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Just small items, looks like we can merge this soon

@@ -1,6 +1,10 @@
using UnityEngine;
using UnityEngine.Scripting;

#if UNITY_IOS && !UNITY_EDITOR
Copy link
Member

Choose a reason for hiding this comment

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

No idea if this works:

Suggested change
#if UNITY_IOS && !UNITY_EDITOR
#define ENABLE_ISO_BRIDGE = UNITY_IOS && !UNITY_EDITOR
#if ENABLE_ISO_BRIDGE

@@ -24,7 +24,7 @@ public static void OnPostProcessBuild(BuildTarget target, string pathToProject)
return;
}

if (!options!.IOSNativeSupportEnabled)
if (!options!.IosNativeSupportEnabled)
Copy link
Member

Choose a reason for hiding this comment

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

Event though "we know it's checked in Validate" it's hard to tell from reading here.
So not against having the null check in the extension method but it looks a bit magic or possibly looks like a bug from the outside. So better have a null check before the use of options here

string? extraValue = null;
if (value is not null)
{
extraValue = SerializeExtraValue(value);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extraValue = SerializeExtraValue(value);
extraValue = SerializeExtraValue(value);
if (extraValue is null)
{
// Failed to serialize, so lets just give up
return;
}

@@ -45,7 +45,7 @@ internal static string GetConfigPath(string? notDefaultConfigName = null)
[field: SerializeField] internal int ShutdownTimeout { get; set; }
[field: SerializeField] internal int MaxQueueItems { get; set; }

[field: SerializeField] internal bool IOSNativeSupportEnabled { get; set; }
[field: SerializeField] internal bool IosNativeSupportEnabled { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a new one bool? and make this one [Obsolete] and read from the old one so we can avoid losing the value when the users upgrade.

[Test]
public void GetTimestamp_ReturnStringConformsToISO8601()
{
var timestamp = SystemClock.Clock.GetUtcNow();
Copy link
Member

Choose a reason for hiding this comment

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

SystemClock is used when we need to control which time is used at different places for testing.
Here you just need a timestamp, better to stick to the framework's API:

Suggested change
var timestamp = SystemClock.Clock.GetUtcNow();
var timestamp = DateTimeOffset.UtcNow;


var actualValue = sut.SerializeExtraValue(new SerializationTestClass());

Assert.AreEqual(null, actualValue);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Assert.AreEqual(null, actualValue);
Assert.IsNull(actualValue);

@bruno-garcia bruno-garcia merged commit 3f72ecd into main Sep 18, 2021
@bruno-garcia bruno-garcia deleted the feat/ios-native-bridge branch September 18, 2021 21:19
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Just to confirm the Objective-C code LGTM.

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