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

Dependency cycle between micrometer-core and reactor-netty caused by ReactorNettySender #2802

Closed
shakuzen opened this issue Oct 5, 2021 · 1 comment
Assignees
Labels
bug A general bug module: micrometer-core An issue that is related to our core module release notes Noteworthy change to call out in the release notes
Milestone

Comments

@shakuzen
Copy link
Member

shakuzen commented Oct 5, 2021

Describe the bug
We have a dependency cycle caused by having the ReactorNettySender implementation in micrometer-core, which forces an optional dependency on reactor-netty. For it's own metrics instrumentation, reactor-netty has an optional dependency on micrometer-core. This is a cycle, with which we've mostly avoided problems by not using new API in either place and so there's no need to upgrade to the latest version. This is not a viable long-term strategy, however.

Environment

  • Micrometer version: any since 1.1.0
  • Micrometer module: micrometer-core
  • OS: N/A
  • Java version: N/A

Expected behavior
No dependency cycles and a clear expectation of which project needs to release first based on non-cyclic dependencies.

Potential solutions
ReactorNettySender is the only class in micrometer-core that depends on reactor-netty. What might we do with it?

  • Deprecate for removal with no replacement
  • Deprecate for removal with the code moved to reactor-netty

For the first option, it's not clear how much ReactorNettySender would be missed if we tried to get rid of it. It is not the default HttpSender implementation, and it requires reactor-netty. Furthermore, to abide by the HttpSender API that requires returning a Response in the send method, it uses the anti-pattern of calling the block method on the reactor-netty HttpClient. I don't know if this is a problem in practice given the limited usage of the sender in Micrometer. We can't easily switch to idiomatic reactor usage because of the API of HttpSender and the point of this issue being to avoid a dependency on reactor in micrometer-core.

Moving the ReactorNettySender class to Reactor Netty would be a way to allow continued use of it while breaking the current cycle.

Additional context
As we are preparing for Micrometer 2.0 development, this dependency cycle becomes a critical issue, since Micrometer or Reactor Netty needs to release first for the other to depend on it, but both can't happen with a dependency cycle.

Separately from the micrometer-core ↔ reactor-netty cycle, we have another cycle because micrometer-registry-statsd has a shaded dependency on reactor-netty and is released together with micrometer-core in this same repository. This issue is focused on only the cycle directly in micrometer-core.

@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module labels Oct 5, 2021
@shakuzen shakuzen added this to the 1.8 tentative milestone Oct 5, 2021
shakuzen added a commit to shakuzen/micrometer that referenced this issue Oct 13, 2021
Having code in micrometer-core that depends on reactor-netty creates a dependency cycle between the two projects. In an effort to work towards eliminating that cycle, we are proactively deprecating the ReactorNettySender. If we receive feedback that users have use cases for it, we can consider moving it to the reactor-netty project or elsewhere. Lacking enough feedback, we will plan to remove this in the next feature release.

See micrometer-metricsgh-2802
@shakuzen shakuzen modified the milestones: 1.8 tentative, 1.9 backlog Oct 13, 2021
jonatan-ivanov pushed a commit that referenced this issue Oct 13, 2021
Having code in micrometer-core that depends on reactor-netty creates a dependency cycle between the two projects. In an effort to work towards eliminating that cycle, we are proactively deprecating the ReactorNettySender. If we receive feedback that users have use cases for it, we can consider moving it to the reactor-netty project or elsewhere. Lacking enough feedback, we will plan to remove this in the next feature release.

See gh-2802
@shakuzen shakuzen modified the milestones: 1.9 backlog, 1.9.0-M1 Nov 16, 2021
@shakuzen shakuzen self-assigned this Nov 16, 2021
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.9.0-M1, 1.9.0-M2 Dec 8, 2021
@shakuzen shakuzen added the release notes Noteworthy change to call out in the release notes label Dec 17, 2021
@shakuzen
Copy link
Member Author

Missed a dependency on reactor-netty-http in the tests for the micrometer-test module when removing related classes.

@shakuzen shakuzen reopened this Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug module: micrometer-core An issue that is related to our core module release notes Noteworthy change to call out in the release notes
Projects
None yet
Development

No branches or pull requests

2 participants