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

Add Helper Function to Build classifierF (to pass to client and server's Middleware) #3167

Merged
merged 42 commits into from Feb 19, 2020

Conversation

kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Feb 9, 2020

Adds a function, classifierFMethodWithOptionallyExcludedPath, to Prometheus.

Given an exclude function, return a 'classifier' function, i.e. for application in
[[org.http4s.client.middleware.Metrics#apply]] and [[org.http4s.server.middleware.Metrics#apply]].

Let's say you want a classifier that excludes integers since your paths consist of:

    * GET    /users/{integer}    = get_users_*
    * POST   /users                = post_users
    * PUT    /users/{integer}    = put_users_*
    * DELETE /users/{integer} = delete_users_*

In such a case, we could use:

   classifierFMethodWithOptionallyExcludedPath[F] { 
         str: String => scala.util.Try(str.toInt).isSuccess 
   }

This idea, namely adding a classifier with an exclude: String => Boolean, was inspired by an internal library that @ChristopherDavenport wrote.

Copy link
Member

@rossabaker rossabaker left a comment

Summoning @ChristopherDavenport, since I think he does the most with metrics.

def classifierFMethodWithOptionallyExcludedPath[F[_]](
exclude: String => Boolean
): Request[F] => Option[String] = { request: Request[F] =>
val dsl: Http4sDsl[F] = Http4sDsl[F]
Copy link
Member

@rossabaker rossabaker Feb 9, 2020

Choose a reason for hiding this comment

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

I'm surprised http4s-dsl is available in this project. That seems like a bad coupling. Can we do this without it?

Copy link
Contributor Author

@kevinmeredith kevinmeredith Feb 9, 2020

Choose a reason for hiding this comment

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

I'm surprised http4s-dsl is available in this project. That seems like a bad coupling.

I only used it since I saw it available via https://github.com/http4s/http4s/blob/series/0.20/build.sbt#L153.

Can we do this without it?

Sure. Can you please advise if you know, off-hand, how to get the path as a List[String], @rossabaker?

Copy link
Member

@rossabaker rossabaker Feb 10, 2020

Choose a reason for hiding this comment

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

You could copy this code and build a List rather than a Path.

In the long run, turning a path into a list of segments ought to be core functionality on a URI.

Copy link
Member

@rossabaker rossabaker Feb 10, 2020

Choose a reason for hiding this comment

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

So this dependency already exists, to support the PrometheusExportService. I think we could re-express it without that, but that's a separate ticket.

Copy link
Contributor Author

@kevinmeredith kevinmeredith Feb 10, 2020

Choose a reason for hiding this comment

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

Thanks, Ross, for telling me how to get a path list from a String. I did that, as well as removed the Http4sDsl usage via commits at #3167 (commits).

@rossabaker rossabaker added the enhancement label Feb 9, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 9, 2020

Are you needing this in 0.20.x?

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 9, 2020

Are you needing this in 0.20.x?

I would not say need, but it'd be nice. If it's a pain to support it in 0.20.x, please let me know, Ross.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 10, 2020

I'd like to phase out 0.20.x releases in general and focus on keeping 0.21 healthy and developing the next version, but if somebody sends PRs, there's no particular reason we can't release another 0.20.

Copy link
Member

@rossabaker rossabaker left a comment

Comments below. This should still pass MiMa checks and be eligible for 0.20.18 and/or 0.21.1.

def classifierFMethodWithOptionallyExcludedPath[F[_]](
exclude: String => Boolean
): Request[F] => Option[String] = { request: Request[F] =>
val dsl: Http4sDsl[F] = Http4sDsl[F]
Copy link
Member

@rossabaker rossabaker Feb 10, 2020

Choose a reason for hiding this comment

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

You could copy this code and build a List rather than a Path.

In the long run, turning a path into a list of segments ought to be core functionality on a URI.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 10, 2020

This should still pass MiMa checks and be eligible for 0.20.18 and/or 0.21.1.

Understood, @rossabaker. I ran mimaReportBinaryIssues from project root successfully. I'll check the build's result tomorrow.

I believe that I've addressed your review findings, Ross. Can you please take a look?

I'd like to phase out 0.20.x releases in general and focus on keeping 0.21 healthy and developing the next version, but if somebody sends PRs, there's no particular reason we can't release another 0.20.

I interpret this to mean that it's OK for me to target series/0.20, but let me know, please, if I understood wrongly.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 10, 2020

Since the other tests succeeded per https://travis-ci.org/http4s/http4s/builds/648221349?utm_source=github_status&utm_medium=notification, I'm guessing the test failure in https://travis-ci.org/http4s/http4s/jobs/648221350?utm_medium=notification&utm_source=github_status was the result of flakiness.

[info] BlazeClientSpec
[info] Blaze Http1Client should
[info]   + raise error NoConnectionAllowedException if no connections are permitted for key (383 ms)
[info]   + make simple https requests (940 ms)
[info]   + behave and not deadlock (994 ms)
[error]   x behave and not deadlock on failures with parTraverse (30 seconds, 186 ms)
[error]    None is not Some (BlazeClientSpec.scala:160)

As a result, I'll close, and then re-open, the PR to prompt another build.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 10, 2020

Looks like another, I'm guessing, flaky test is to blame per https://travis-ci.org/http4s/http4s/jobs/648223657?utm_medium=notification&utm_source=github_status. I will close, and then re-open.

To make sure I follow, is it necessary to get the build green, @rossabaker, even if you have high confidence that a PR was not to blame for a failing test?

Copy link
Member

@rossabaker rossabaker left a comment

I'm rerunning the test, but the failure is definitely not related to this ticket.

Thanks for the cleanups.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 10, 2020

And yes, if you need this on 0.20.18, it's not a problem to keep it on this branch. I backported the GitHub Actions build and threw in a Tomcat bugfix in separate PRs to make it more worth our while.

It's already included in Http4sSpec.
The motivation for this change was to show that the larger PR did not modify the build.sbt.
The motivation for this change was to show that the larger PR did not modify this file.
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 16, 2020

Can you please review this one, @ChristopherDavenport, again?

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 16, 2020

The most recent build appears to have run into a transient error.

[error] coursier.ResolutionException: 1 download error
[error]     Caught java.nio.channels.OverlappingFileLockException while downloading https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.9.8/jackson-databind-2.9.8.jar
[error] (play-json / update) coursier.ResolutionException: 1 download error
[error]     Caught java.nio.channels.OverlappingFileLockException while downloading https://repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.9.8/jackson-databind-2.9.8.jar
[error] Total time: 299 s (04:59), completed Feb 16, 2020, 4:53:50 AM
##[error]Process completed with exit code 1.

I'll close, and then re-open.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 16, 2020

I see a few tests failed:

[info] Blaze Http1Client should
[info]   + raise error NoConnectionAllowedException if no connections are permitted for key (1 second, 131 ms)
[info]   + make simple https requests (1 second, 158 ms)
[info]   + behave and not deadlock (1 second, 182 ms)
[error]   x behave and not deadlock on failures with parTraverse (30 seconds, 576 ms)
[error]    None is not Some (BlazeClientSpec.scala:160)
[error] org.http4s.client.blaze.BlazeClientSpec.$anonfun$new$21(BlazeClientSpec.scala:160)
[info]   + behave and not deadlock on failures with parSequence (201 ms)
[info]   + obey response header timeout (212 ms)
[info]   + unblock waiting connections (1 second, 55 ms)
[info]   + reset request timeout (2 seconds, 57 ms)
[info]   + drain waiting connections after shutdown (1 second, 139 ms)
[info]   + cancel infinite request on completion (1 second, 151 ms)
[info]   + doesn't leak connection on timeout (1 second, 161 ms)
[error]   x call a second host after reusing connections on a first (5 seconds, 68 ms)
[error]    Some(Left(())) != Some(Right(simple path)) (BlazeClientSpec.scala:324)
[error] org.http4s.client.blaze.BlazeClientSpec.$anonfun$new$82(BlazeClientSpec.scala:324)
[error] Actual:   Some(Left(()))
[error] Expected: Some(Right(simple path))
[info]   + raise a ConnectionFailure when a host can't be resolved (26 ms)
[info] Total for specification BlazeClientSpec
[info] Finished in 36 seconds, 859 ms
[info] 13 examples, 18 expectations, 2 failures, 0 error
[error] Failed: Total 66, Failed 2, Errors 0, Passed 61, Skipped 3, Pending 1

but I highly doubt that my changes introduced such tests' failures.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 17, 2020

Yes, three are still a few tests that are flaky. I was hoping the move to GitHub Actions would help, and, well, maybe, but not entirely.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Feb 18, 2020

Hi @rossabaker - please let me know what I can do to influence the merging of this PR. Thanks.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Feb 19, 2020

@ChristopherDavenport, are all your comments addressed?

@rossabaker rossabaker merged commit c253f05 into http4s:series/0.20 Feb 19, 2020
19 of 22 checks passed
@kevinmeredith kevinmeredith deleted the classifier-helper branch Feb 23, 2020
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 10, 2020

Hi @rossabaker and @ChristopherDavenport - could you please cut a release per v0.20.19...series/0.20? I don't have a rush, but I'd like to start using this function. Thanks!

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 19, 2020

Could you please take a look at #3167 (comment), Ross or Chris?

I believe I have a use case for this in prod today.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Mar 20, 2020

I was trying to get another fix in, but life has been upside down. Let's get this out now. Versions are cheap.

@rossabaker rossabaker mentioned this pull request Mar 20, 2020
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 20, 2020

I was trying to get another fix in, but life has been upside down. Let's get this out now. Versions are cheap.

Thank you, @rossabaker !

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 20, 2020

I was trying to get another fix in, but life has been upside down

Yes, unfortunately so. I'm glad to have spoken with you last week at NEScala. I hope you're doing well.

rossabaker added a commit to http4s/http4s-servlet that referenced this issue Apr 3, 2022
…elper

Add Helper Function to Build classifierF (to pass to client and server's Middleware)
armanbilge pushed a commit to http4s/http4s-prometheus-metrics that referenced this issue May 2, 2022
…elper

Add Helper Function to Build classifierF (to pass to client and server's Middleware)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants