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

merge telemetry module into telemetry-core to help reduce split modul… #269

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

jack-berg
Copy link
Contributor

@jack-berg jack-berg commented Jan 22, 2021

An alternative and less intrusive solution to the split module issues discussed in PR 267.

Instead of qualifying the packages of telemetry, telemetry-core, telemetry-http-java11 and telemetry-http-okhttp, this merges the telemetry module into telemetry-core, keeping all classes in their same package to reduce burden on consumers.

This won't completely fix the issues discussed: you can still run into the split module issue since telemetry-http-java11 and telemetry-http-okhttp still place their code in the same com.newrelic.telemetry package as telemetry-core. However, its fairly trivial to re-implement the implementations of HttpPoster that those modules provide.

We should consider qualifying the package names in telemetry-http-okhttp and telemetry-http-java11 in the future, which would fully solve the issue. However, that would be a breaking change for consumers that would require further discussion. Granted, it would be straight forward for consumers to adapt to, given the small surface area of those modules.

@XiXiaPdx
Copy link
Contributor

XiXiaPdx commented Jan 26, 2021

Ok, I think I'm seeing this now.

  1. End user wants to use the SDK in their modularized application.
  2. Their application is created with a module-info, so it is a named module.
  3. The application being a named module, it will not be able to access the telemetry SDK classes because the SDK's packages become part of the Unnamed module.
  4. Named modules can not read from the Unnamed module (unless --add-reads added to java command or through instrumentation)

Named modules can read Automatic modules

  1. Make the SDK jars into automatic modules.
  • add an attribute to the jar manifest 'Automatic-Module-Name' (optional)
  • place the jars on the module-path

Automatic modules export all of their packages so Named modules can read them. This creates a the Split Packages problem.

"...no two modules are allowed to even contain the same package (exported or not)."

As pointed out in PR 267, the SDK's various modules and their classes all currently are under one package com.newrelic.telemetry. So when they become automatic modules, they all contain and export the same package. No go here.

And as the PR title says, by merging two modules into one, we reduce our split package problem a bit.

Copy link
Contributor

@XiXiaPdx XiXiaPdx left a comment

Choose a reason for hiding this comment

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

For the 5 classes that are moving, do you think there is any benefit to organizing them into their own directory in the core?

@XiXiaPdx
Copy link
Contributor

I see the current full name com.newrelic.telemetry.TelemetryClient; in two examples that would need to be updated.

@kittylyst
Copy link
Contributor

I see the current full name com.newrelic.telemetry.TelemetryClient; in two examples that would need to be updated.

Moving API classes to different packages would very much be a breaking change - and I don't think we can't do that.

@jack-berg
Copy link
Contributor Author

For the 5 classes that are moving, do you think there is any benefit to organizing them into their own directory in the core?

Do you mean a separate source directory? Or do you mean a separate package?

If you mean a separate source directory, I don't think there's any benefit to that.

If you mean a separate package, my original PR 267 did just that - instead of merging the modules, it further qualified the root package of each of the modules such that they didn't conflict.

If this was a new project, I'd say that's the approach we should take. However, given that would be a breaking change for projects dependent on the library, I think that the approach suggested in this PR nicely balances the concerns: it fixes the big split module issue folks will run into while only requiring dependents to remove a dependency declaration from their build in order to upgrade.

@jack-berg jack-berg merged commit 606aaa0 into main Jan 26, 2021
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.

3 participants