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

chore: Remove private references from public headers #2743

Merged
merged 12 commits into from
Mar 20, 2023

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Mar 2, 2023

📜 Description

We made SentrySession private but we did not remove references from public headers.

Also changing a few forward declarations to #import to help with .NET hybrid SDK.

This may be considered a "breaking change" since we're removing property from a public header, but is a property that was not meant to be used.

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

#skip-changelog

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.35 ms 1239.62 ms 14.27 ms
Size 20.76 KiB 425.71 KiB 404.95 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8f397a7 1224.66 ms 1236.48 ms 11.82 ms
ff09c7e 1240.94 ms 1262.66 ms 21.72 ms
407ff99 1225.49 ms 1232.88 ms 7.39 ms
bd2afa6 1192.31 ms 1210.37 ms 18.05 ms
8f397a7 1196.55 ms 1226.82 ms 30.27 ms
49819af 1263.92 ms 1275.66 ms 11.74 ms
8e76be4 1272.67 ms 1286.38 ms 13.71 ms
ad7cec6 1203.22 ms 1224.74 ms 21.52 ms
e71cf92 1201.69 ms 1226.52 ms 24.83 ms
20163bb 1248.92 ms 1258.48 ms 9.56 ms

App size

Revision Plain With Sentry Diff
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
ff09c7e 20.76 KiB 427.76 KiB 407.00 KiB
407ff99 20.76 KiB 427.87 KiB 407.10 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
49819af 20.76 KiB 427.31 KiB 406.55 KiB
8e76be4 20.76 KiB 427.66 KiB 406.89 KiB
ad7cec6 20.76 KiB 427.32 KiB 406.55 KiB
e71cf92 20.76 KiB 419.85 KiB 399.10 KiB
20163bb 20.76 KiB 426.82 KiB 406.06 KiB

Previous results on branch: fix/remove-private-references

Startup times

Revision Plain With Sentry Diff
1e62fb9 1243.94 ms 1257.08 ms 13.14 ms
f536f5b 1254.43 ms 1261.58 ms 7.15 ms
01048ab 1241.26 ms 1257.50 ms 16.24 ms

App size

Revision Plain With Sentry Diff
1e62fb9 20.76 KiB 426.31 KiB 405.55 KiB
f536f5b 20.76 KiB 427.25 KiB 406.49 KiB
01048ab 20.76 KiB 426.32 KiB 405.55 KiB

@brustolin brustolin marked this pull request as ready for review March 2, 2023 16:56
@armcknight
Copy link
Member

@brustolin could you link to some context around how the forward class declarations were causing problems in the .NET SDK?

@brustolin
Copy link
Contributor Author

@armcknight This is the PR that started the discussion.
Matt did a workaround with a script, but that is not the optimal solution.

@brustolin
Copy link
Contributor Author

I strongly believe that forward declaration should be only used to avoid circular reference.
If a class strongly depends on a second one, an import is the best approach to avoid the need to import a bunch of headers in the implementation file of a third class that wants to use the first one.

@armcknight
Copy link
Member

@armcknight This is the PR that started the discussion.
Matt did a workaround with a script, but that is not the optimal solution.

Hmm, I'm not seeing discussion about any issues around this. Does this comment in the linked issue allude to the problem? getsentry/sentry-dotnet#2148 (comment)

@armcknight
Copy link
Member

armcknight commented Mar 8, 2023

I strongly believe that forward declaration should be only used to avoid circular reference.
If a class strongly depends on a second one, an import is the best approach to avoid the need to import a bunch of headers in the implementation file of a third class that wants to use the first one.

This is an unorthodox view, and one I disagree with based on experience. It is not fun to disentangle a chain of implicitly imported headers when you finally do hit a circular reference. I understand it can be a pain with all the imports in an implementation file, but that's a good sign that it's time to rethink the design of the implementation and the things its using.

Much has been written about this, here is a good article that talks about the tradeoffs: https://qualitycoding.org/file-dependencies/

@brustolin
Copy link
Contributor Author

brustolin commented Mar 8, 2023

I'm not seeing discussion

Oh, sorry, the discussion happened in slack. Short version, the C# tooling used to creating binding to objc classes don't now what to do with a forward declaration if the header is not included or does not have a reference anywhere.

This is an unorthodox view, and one I disagree with based on experience, It is not fun to disentangle a chain of implicitly imported headers when you finally do hit a circular reference.

By experience I would say is much better avoiding cyclic reference in classes itself, then you don't need to deal with header cyclic reference. If it is hard to "disentangle a chain of implicitly imported headers when you finally do hit a circular reference" that's a good sign that it's time to rethink the project design.

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

We should set up integration tests for .NET/RN/Flutter so we can make sure we're not breaking things there. This is some weird, complicated stuff that I won't pretend to understand 😂

Sources/Sentry/include/SentryTraceContext.h Outdated Show resolved Hide resolved
@brustolin
Copy link
Contributor Author

We should set up integration tests for .NET/RN/Flutter so we can make sure we're not breaking things there. This is some weird, complicated stuff that I won't pretend to understand 😂

Yes, we have an issue for this already.

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.

Thanks for doing this cleanup, @brustolin. I have two questions.

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.

LGTM

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #2743 (cc3cb0c) into main (dacf894) will increase coverage by 0.74%.
The diff coverage is n/a.

❗ Current head cc3cb0c differs from pull request most recent head 5cbeb5a. Consider uploading reports for the commit 5cbeb5a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2743      +/-   ##
==========================================
+ Coverage   80.55%   81.30%   +0.74%     
==========================================
  Files         249      258       +9     
  Lines       23117    24117    +1000     
  Branches    10256    10703     +447     
==========================================
+ Hits        18623    19609     +986     
+ Misses       4021     4009      -12     
- Partials      473      499      +26     
Impacted Files Coverage Δ
Sources/Sentry/SentryHub.m 95.98% <ø> (ø)
Sources/Sentry/SentryTraceContext.m 91.20% <ø> (ø)

... and 44 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dacf894...5cbeb5a. Read the comment docs.

@brustolin brustolin merged commit d80d410 into main Mar 20, 2023
@brustolin brustolin deleted the fix/remove-private-references branch March 20, 2023 12:24
@mattjohnsonpint
Copy link
Contributor

FYI, This made SentrySdkInfo and SentryTraceContext available, but now I have the same issue with SentryBaggage and SentryTraceHeader. I still have it with SentrySession as before.

Now that I know how to make type-forwarded stubs in .NET, it's not a huge deal - but I thought you'd want to know.

See PrivateApiDefinitions.cs in getsentry/sentry-dotnet@77a69c5#diff-fbdac032f98e5bb7b38c5933992d8ae94ab4e29c4472e092f7acb01cecc1b3bb

@brustolin
Copy link
Contributor Author

@mattjohnsonpint It looks like we gonna have to solve this with C# tooling. If we keep rearranging header, most likely other headers will became an issue.

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.

5 participants