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(performance): make ActivityFramesTracker public #1922

Closed
wants to merge 5 commits into from

Conversation

ninniuz
Copy link

@ninniuz ninniuz commented Feb 27, 2022

📜 Description

Make ActivityFramesTracker and LoadClass public.

💡 Motivation and Context

If the Context used in SentryAndroid.init is not an Application then the ActivityFramesTracker integration is not automatically added to the available ones.
It might be possible that applications need to init Sentry in Context#attachBaseContext providing base Context instead of the current Application.

With this change, integrators can enable ActivityFramesTracker integration during configuration init.

💚 How did you test it?

I have created two variants of the sample Android app and verified them both against a personal sentry.io project.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

Maybe add a no-args ctor to ActivityFramesTracker which internally provisions an instance of LoadClass? It feels a bit awkward to have to create that class in the ctor.

If the Context used in SentryAndroid.init is not an Application
then the ActivityFramesTracker integration is not automatically
added to the available ones.
It might be possible that applications need to init Sentry in
Context#attachBaseContext providing base Context instead of the
current Application.

With this change, integrators can enable ActivityFramesTracker
integration during configuration init.
@codecov-commenter
Copy link

Codecov Report

Merging #1922 (274bf25) into main (4362229) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1922   +/-   ##
=========================================
  Coverage     75.46%   75.46%           
  Complexity     2248     2248           
=========================================
  Files           225      225           
  Lines          8036     8036           
  Branches        851      851           
=========================================
  Hits           6064     6064           
  Misses         1562     1562           
  Partials        410      410           

Continue to review full report at Codecov.

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

Comment on lines +87 to +100
flavorDimensions += "init"
productFlavors {
create("default") {
dimension = "init"
applicationIdSuffix = ".default"
versionNameSuffix = "-default"
}
create("early") {
dimension = "init"
applicationIdSuffix = ".early"
versionNameSuffix = "-early"
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
flavorDimensions += "init"
productFlavors {
create("default") {
dimension = "init"
applicationIdSuffix = ".default"
versionNameSuffix = "-default"
}
create("early") {
dimension = "init"
applicationIdSuffix = ".early"
versionNameSuffix = "-early"
}
}
flavorDimensions += "init"
productFlavors {
create("default") {
dimension = "init"
applicationIdSuffix = ".default"
versionNameSuffix = "-default"
}
create("early") {
dimension = "init"
applicationIdSuffix = ".early"
versionNameSuffix = "-early"
}
}

I'd rather remove the flavors since it makes the build slower, with more stuff to build and test, per flavor.

@@ -0,0 +1,53 @@
package io.sentry.samples.android;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather roll back the sample changes, and focus only on the classes visibility.

@marandaneto
Copy link
Contributor

@ninniuz please add an entry to the changelog :)

Thanks for doing this.

@marandaneto
Copy link
Contributor

@ninniuz Thanks for doing the PR, closing it in favor of #1931 anyway since we need it asap.
For some reason, git failed pushing to your branch, so I could address the view myself.

@marandaneto marandaneto closed this Mar 2, 2022
@ninniuz
Copy link
Author

ninniuz commented Mar 2, 2022

@marandaneto 👍

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.

None yet

3 participants