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

Extract modules from core #711

Merged

Conversation

surbhiia
Copy link
Contributor

@surbhiia surbhiia commented Nov 27, 2024

As a first step towards #669 , I've extracted following 3 modules from core:

:common

  • Holds classes which are required in :core, :services and some instrumentations too like RumConstants.
  • Not published so keeping it internal. Added as implementation dependency everywhere.
  • Had to extract networkattrs->data and CurrentNetworkAttributesExtarctor here as it is utilized by both :services in CurrentNetworkProvider and :core in NetworkAttributesSpanAppender.
  • Extracted RuntimeDetailsExtractor used by crash instrumentation here.

:services

  • The services folder in core is extracted as a module
  • Moved SystemTime class here as well as it is a good contender for a shared utility. It is currently only used by the PeriodicWorkService. Alternatively, We can move it to :common module (Update - moved to :common now)
  • Not published so keeping it internal. Also used internal keyword in package name. Added as implementation dependency everywhere.

:instrumentation:android-instrumentation

  • AndroidInstrumentation AutoService interface and related classes
  • This is published as currently we're allowing users to bring in their own custom instrumentations that implement this interface and add via the addInstrumentation() API we have exposed. Added as api dependency everywhere right now.

Other callouts:

  • Preferences.java in service module depended on the generated BuildConfig.java for LIBRARY_PACKAGE_NAME for getting the shared preference object. It does not make sense to generate BuildConfig.java in any other module other than :core as it is project specific. So, I've replaced the dependence by direct replacement with "io.opentelemetry.android" string. Also, it did not make sense to add this string to RumConstants and use from there.
  • test/resources/META-INF/services and TestAndroidInstrumentation is replicated right now in :core and :instrumentation:android-instrumentation modules as it is just providing an AndroidInstrumentation AutoService on the classpath for tests in both modules as those tests are specific to the individual modules (AgentInitTest, OpentelemetryRumBuilderTest, AndroidInstrumentationLoaderImplTest). (While writing this, it comes to my mind that we can perhaps move TestAndroidInstrumentation as a shared test class in test-common module.)

This is just a first step, we've a long way to go in determining the future possibilities of how we can support instrumentations to be used independently of this SDKs core (initializing and configuring) module. Those discussions are covered in following issues - #658, #669 , #702.

Screenshots of the new modules structure from android studio project explorer:
Screenshot 2024-11-27 at 3 37 16 PM
Screenshot 2024-11-27 at 3 36 43 PM
Screenshot 2024-11-27 at 3 35 53 PM

@surbhiia surbhiia requested a review from a team as a code owner November 27, 2024 23:26
Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Thanks! I think it's a good start. I do believe we should move SystemTime to common though, as you mentioned:

Moved SystemTime class here as well as it is a good contender for a shared utility. It is currently only used by the PeriodicWorkService. Alternatively, We can move it to :common module

It's pretty common having to use the current time and it's tricky to test those use cases without something like SystemTime. Here's an example of a usage outside of the services scope where having it in common should help.

Other than that I think we can move forward with these changes, though I'll leave this PR open until next week to give a chance for people in the US to have a look too, as I've noticed that this week has been pretty quiet in OTel in general due to the holidays.

@surbhiia surbhiia force-pushed the develop/extract-modules-from-core branch from f27815a to a49ff87 Compare December 3, 2024 03:56
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

This feels like progress. Thanks for contributing this!

@breedx-splk breedx-splk merged commit 015f8ed into open-telemetry:main Dec 5, 2024
3 checks passed
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