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

HttpRequestTags depends on both jakarta and javax Servlet variants #3804

Closed
countableSet opened this issue May 2, 2023 · 5 comments
Closed
Assignees
Labels
breaking-change bug A general bug module: micrometer-core An issue that is related to our core module
Milestone

Comments

@countableSet
Copy link

Describe the bug

When javax.* dependency isn't included in the project, the class io.micrometer.core.instrument.binder.http.HttpRequestTags fails to compile when using implementation("io.micrometer:micrometer-jetty11:$micrometerVersion") version of TimeHandler.

Environment

  • Micrometer version 1.10.6 and main branch
  • Micrometer registry [e.g. prometheus] statsd but more specifically jetty11 jar
  • OS: macOS 13.3.1
  • Java version: 17
openjdk version "17.0.6" 2023-01-17 LTS
OpenJDK Runtime Environment Zulu17.40+19-CA (build 17.0.6+10-LTS)
OpenJDK 64-Bit Server VM Zulu17.40+19-CA (build 17.0.6+10-LTS, mixed mode, sharing)

To Reproduce
How to reproduce the bug:

I added a test case to the io.micrometer.jetty11.TimedHandlerTest (see branch) which reproduces the issue.

./gradlew :mirometer-jetty11:test
[...]
> Task :micrometer-jetty11:compileTestJava FAILED
/micrometer/micrometer-jetty11/src/test/java/io/micrometer/jetty11/TimedHandlerTest.java:4
26: error: cannot access HttpServletRequest
                HttpRequestTags.method(request),
                               ^
  class file for javax.servlet.http.HttpServletRequest not found
/micrometer/micrometer-jetty11/src/test/java/io/micrometer/jetty11/TimedHandlerTest.java:427: error: cannot access HttpServletResponse
                HttpRequestTags.status(response),
                               ^
  class file for javax.servlet.http.HttpServletResponse not found
/micrometer/micrometer-jetty11/src/test/java/io/micrometer/jetty11/TimedHandlerTest.java:428: error: cannot access HttpServletResponse
                HttpRequestTags.outcome(response)));
                               ^
  class file for javax.servlet.http.HttpServletResponse not found
3 errors

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':micrometer-jetty11:compileTestJava'.
> Compilation failed; see the compiler error output for details.

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/8.1.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 2s
59 actionable tasks: 53 executed, 6 from cache

Expected behavior
Should compile without issues in a project without javax.* dependencies.

Additional context

Tried with a lambda:

var handler = new TimedHandler(registry, Tags.empty(),
        (request, response) -> Tags.of(
            Tag.of("path", request.getPathInfo()),
            HttpRequestTags.method(request),
            HttpRequestTags.status(response),
            HttpRequestTags.outcome(response)));

and custom class

public class CustomHttpJakartaServletRequestTagsProvider implements
    HttpJakartaServletRequestTagsProvider {

  @Override
  public Iterable<Tag> getTags(HttpServletRequest request, HttpServletResponse response) {
    return Tags.of(
        Tag.of("path", request.getPathInfo()),
        HttpRequestTags.method(request),
        HttpRequestTags.status(response),
        HttpRequestTags.outcome(response));
  }
}

with the same problem.

However, using the constructor without the provider works fine.

var timedHandler = new TimedHandler(registry, Tags.empty());
@bclozel
Copy link
Contributor

bclozel commented May 3, 2023

@countableSet the HttpRequestTags implementation is bound to the javax.* Servlet API variant. Jetty 11 depends on Servlet 5.0 which is using the jakarta.* variant. This is why the TimedHandler requires a HttpJakartaServletRequestTagsProvider (like DefaultHttpJakartaServletRequestTagsProvider) to work.

I believe that replacing HttpRequestTags with DefaultHttpJakartaServletRequestTagsProvider in your test should work.

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2023
@bclozel bclozel added invalid An issue that we don't feel is valid and removed waiting-for-triage labels May 3, 2023
@countableSet
Copy link
Author

@bclozel I might be misunderstanding you but I am using the HttpJakartaServletRequestTagsProvider interface and if you look at the implementation version of DefaultHttpJakartaServletRequestTagsProvider it calls HttpRequestTags.

public class DefaultHttpJakartaServletRequestTagsProvider implements HttpJakartaServletRequestTagsProvider {

    @Override
    public Iterable<Tag> getTags(HttpServletRequest request, HttpServletResponse response) {
        return Tags.of(HttpRequestTags.method(request), HttpRequestTags.status(response),
                HttpRequestTags.outcome(response));
    }

}

HttpRequestTags has implementations for jakarta, however, I can't use them because of the javax compile failure. I suppose copy pasta these methods into my project but it is kinda annoying.

    public static Tag method(jakarta.servlet.http.HttpServletRequest request) {
        return (request != null) ? Tag.of("method", request.getMethod()) : METHOD_UNKNOWN;
    }

If you take look at the branch I referenced it adds a test to TimedHandler in the jetty11 project that used a lambda implementation of the HttpJakartaServletRequestTagsProvider which call the jakarata method versions in HttpRequestTags which caused the compiler failure.

Could you please take a second look at the issue? Thanks!

It would be nice to break up the javax and jakarta methods into different class to avoid compiler failures.

@bclozel bclozel changed the title HttpRequestTags fails to compile with jetty11 jakarta without javax dependencies HttpRequestTags depends on both jakarta and javax Servlet variants May 3, 2023
@bclozel
Copy link
Contributor

bclozel commented May 3, 2023

Thanks @countableSet I missed that part. In this case Jetty is not really central to this issue.

I'll reopen it so we can reconsider this choice.

@bclozel bclozel removed the invalid An issue that we don't feel is valid label May 3, 2023
@bclozel bclozel reopened this May 3, 2023
@countableSet
Copy link
Author

@bclozel thanks so much! I also made an isolated project if that helps here, with different branches for jakarta and javax. I was also curious if the issue happens in reverse, it it does as well.

If you try and use HttpRequestTags class without jakarta.* dependencies, it fails as well with error with a similar error

> Task :compileTestJava FAILED
/micrometer-bug/src/test/java/dev/example/test/TimedHandlerTest.java:66: error: cannot access HttpServletRequest
            HttpRequestTags.method(request),
                           ^
  class file for jakarta.servlet.http.HttpServletRequest not found
/micrometer-bug/src/test/java/dev/example/test/TimedHandlerTest.java:67: error: cannot access HttpServletResponse
            HttpRequestTags.status(response),
                           ^
  class file for jakarta.servlet.http.HttpServletResponse not found
/micrometer-bug/src/test/java/dev/example/test/TimedHandlerTest.java:68: error: cannot access HttpServletResponse
            HttpRequestTags.outcome(response)));
                           ^
  class file for jakarta.servlet.http.HttpServletResponse not found
3 errors

@bclozel bclozel added this to the 1.12 backlog milestone May 5, 2023
@bclozel
Copy link
Contributor

bclozel commented May 5, 2023

Applications should ensure that they rely on javax.* or jakarta.* namespace, but not both. It's a good idea to ensure that only one flavor is part of the application dependencies. Here, HttpRequestTags uses both - loading the class means that all types exposed on the methods are loaded transitively. We should split this arrangement and add a deprecation notice.

I've marked this for the next feature release as it's too late to take care of this now for 1.11.

@bclozel bclozel modified the milestones: 1.12 backlog, 1.12.0-M1 Jun 20, 2023
@bclozel bclozel self-assigned this Jun 20, 2023
@bclozel bclozel added the module: micrometer-core An issue that is related to our core module label Jun 26, 2023
bclozel added a commit to bclozel/micrometer that referenced this issue Jun 26, 2023
Prior to this commit, `HttpRequestTags` would provide Tags for both
"javax.*" and "jakarta.*" variants of `HttpServletRequest`. While the
`HttpServletRequestTagsProvider` and
`HttpJakartaServletRequestTagsProvider` are separate interfaces, they
both use `HttpRequestTags` and import all types exposed on its methods.

It is strongly advised not to import both "javax.*" and "jakarta.*"
variants in a typical application and the current arrangement prevents
this best practice.

This commit deprecates Jakarta methods variants in `HttpRequestTags` and
moves them to `HttpJakartaServletRequestTags`. This allows a clean
dependency setup for Jakarta apps and a migration path for all in the
meantime.

Closes micrometer-metricsgh-3804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change bug A general bug module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants