Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 00:23

    finaglehelper on develop

    finatra: Add validation to proj… (compare)

  • Dec 13 18:50

    finaglehelper on gh-pages

    site push by csl (compare)

  • Dec 13 02:49

    finaglehelper on finatra-19.12.0

    (compare)

  • Dec 13 01:46
    kimxogus commented #511
  • Dec 13 01:34
    ryanoneill commented #511
  • Dec 13 00:28

    finaglehelper on develop

    Twitter-oss: Prepare OSS librar… (compare)

  • Dec 12 19:19

    finaglehelper on develop

    Revert "Upgrade guava to 23.6.1… (compare)

  • Dec 12 05:20

    finaglehelper on develop

    finatra-http: Fix message body … (compare)

  • Dec 11 00:27

    finaglehelper on develop

    finatra: Update Link to Finagle… finatra: Remove Documentation O… (compare)

  • Dec 10 23:37

    finaglehelper on develop

    Upgrade guava to 23.6.1-jre **… (compare)

  • Dec 10 00:36
    ryanoneill commented #511
  • Dec 10 00:23

    finaglehelper on develop

    jackson: Update to 2.9.10 (and … (compare)

  • Dec 06 22:24

    finaglehelper on develop

    finatra/jackson: Remove Pants B… (compare)

  • Dec 06 21:18

    finaglehelper on develop

    finatra filters: Correctly coun… (compare)

  • Dec 06 20:54
    cacoco closed #513
  • Dec 06 20:54
    cacoco commented #513
  • Dec 06 20:34

    finaglehelper on develop

    finatra/doc: Fix typo in gettin… (compare)

  • Dec 06 20:14
    cacoco commented #511
  • Dec 06 20:13
    cacoco commented #511
  • Dec 06 20:13
    cacoco commented #511
Christopher Coco
@cacoco
it's like nothing is tied to the lifecycle and all bets off
the collector register should probably happen in an init
in any case, I don't really know why this blows up in tests but is fine in production other maybe it's a dependency issue then or something else is different between test and production
What happens if you don't use the exporter trait and do what it's doing manually? E.g....
class Main extends HttpServer {

  init {
    PrometheusMetricsCollector().register()
  }

  override val modules: Seq[TwitterModule] = Seq(DbConnectionProviderModule)

  override def configureHttp(router: HttpRouter): Unit = {
    router
      .filter[LoggingMDCFilter[Request, Response]]
      .filter[CommonFilters]
      .filter[CorsFilter]
      .exceptionMapper[EntityNotFoundExceptionMapper]
      .exceptionMapper[MappingExistsMapper]
      .exceptionMapper[OverCapacityExceptionMapper]
      .exceptionMapper[DuplicateOccurrenceExceptionMapper]
      .add[OccurrenceController]
      .add[ZoneController]
  }

  premain {
    addAdminRoute(PrometheusExporter.metricsRoute)
  }

  override protected def warmup(): Unit = {
    handle[Warmup]
  }

  override protected def setup(): Unit = {
    // Run migrations
    val conf: Config = ConfigFactory.load()
    val dbConfig: Config = conf.getConfig("gregor.db")

    Utils
      .buildMigrateConf(dbConfig)
      .locations("com.armoredthings.gregor.database.migration")
      .load()
      .migrate()

    super.setup()
  }
}
Christopher Coco
@cacoco
Ok found an example where it was a requirement in testing to "drop all" the metrics from the sink: https://github.com/twitter/finatra/blob/1b34404068f7f87e062dcdd0d3d2af36bbe04727/examples/hello-world-heroku/src/test/scala/com/twitter/hello/heroku/HelloWorldFeatureTest.scala#L14. Not sure if there is an equivalent here.
sinanspd
@sinanspd
Aaa this is removing it from Finagle's registry rather than Prometheus'. thank you so much. Will give this a shot
Christopher Coco
@cacoco
Sound good
Sladyn
@sladyn98

Hey @sladyn98 sorry, I meant to respond to this earlier. I don’t know the status of of community bridge initiative. Let me ask our open source program office.

@cacoco Hello any updates on this ?

Christopher Coco
@cacoco
@sladyn unfortunately no. Things are apparently in flux at the moment
sinanspd
@sinanspd

@cacoco looked into this a little bit more.

1) The error is directly coming from Prometheus.register() that is inside the trait.
2) The lifecycle approach doesn't work either, however is there a way to override the lifecycle hooks? If I was able to do that, I can init prometheus on premain in the application but override it in the EmbeddedHttpServer. Wasn't able to locate a way of doing this tho
3) On that note

import com.twitter.finagle.metrics._

seems to be deprecated. I was able to locate MetricsStatsReceiver in

com.twitter.finagle.stats.MetricsStatsReceiver

but it doesn't seem to expose those .metrics. Is there a corresponding version of this in the most recent finagle ?

Christopher Coco
@cacoco
@sinanspd you don’t override lifecycle methods in the embedded http sever it is a wrapper over a server under test. You do it in the instance your testing. The lifecycle approach is more correct than the example given and should have the same affect but with the benefit of actually being able to use flags if necessary since their values are parsed in between init and Premain. Lastly the example was someone else integration and not Finagle and Finagle doesn’t expose the same thing for reasons.
Looking at the error you need a way to clear metrics but I don’t know how much help we’re going to be since we’re not Prometheus experts. I saw you flagged a ticket for them feel free to add me to it
sinanspd
@sinanspd
gotcha. I was trying to see if we can get to the bottom of this on the finatra side rather than prometheus. Thanks for the insight. Something like this works as far as I can see,
premain {
    s.get match {
      case None =>
        PrometheusMetricsCollector().register()
        addAdminRoute(Route.isolate(Route(
          path = "/metrics",
          handler = new PrometheusMetricsExporterService(),
          alias = "Prometheus Metrics",
          group = Some(Grouping.Metrics),
          includeInIndex = true
        )
        ))
      case Some("prod") =>
        PrometheusMetricsCollector().register()
        addAdminRoute(Route.isolate(Route(
          path = "/metrics",
          handler = new PrometheusMetricsExporterService(),
          alias = "Prometheus Metrics",
          group = Some(Grouping.Metrics),
          includeInIndex = true
        )
        ))
      case Some(r) => r
    }
  }
I think the problem is them using the default registry, I will try to write up a PR to specify registries in the library and see if they would be willing to merge that
thanks for all the help
Christopher Coco
@cacoco
Fwiw, it would be great if they were not touching the registry directly but using the LoadedStatsReceiver
sinanspd
@sinanspd
noted. I will shape the PR around that
Christopher Coco
@cacoco
That way you wouldn’t need a custom exporter...but agin I don’t know the intentions here
Edward Cernera
@cernerae-avlino

Referencing bug: twitter/finatra#514

Migrated from finatra-19.7.0 to 19.11.0, now the Controller will not resolve endpoints with parameters, like /api/hello/:status/

Moses Nakamura
@mosesn
@cernerae-avlino thanks for filing a ticket! It’s a holiday in the US tomorrow, and we’re taking off Friday too, so it’s unlikely that we’ll get to it until next week.
just to set expectations appropriately. this is probably related to us changing our routing backend, thanks for the reproduction case!
Christopher Coco
@cacoco
@cernerae-avlino Thanks for the ticket. FWIW, it is documented that you should list routes in order of specificity. The wildcard "*" route seems to be listed first and thus matches first. The wild card is less specific here.
And this should be later
Edward Cernera
@cernerae-avlino
@cacoco Understood. My comment to the ticket is more specific to the issue I'm referring to (https://github.com/twitter/finatra/issues/514#issuecomment-559194608), which is the param not being picked up. Thought it'd be more beneficial to piggy back off the related ticket.
Christopher Coco
@cacoco
I am dubious of this being an issue as we have plenty of tests around that case and many internal usages so am wondering if there is something else awry. What version were you upgrading from since you confirmed you saw the behavior in 19.7.0? That is what is the version that worked for you?
Hmm, you're doing this inside of a prefix, perhaps that is adding some wrinkle. We'll have to diagnose.
It would be great if you didn't piggyback on this ticket as it looks like you are experiencing something different and it would be great if you opened a separate ticket.
Edward Cernera
@cernerae-avlino

Was using 19.7.0 for several months, the routing was working as expected; I am also utilizing the prefixes. I just upgraded to 19.11.0 on @jyanJing 's recommendation to get the streaming updates:

Hi @mehulmistryavlino , unfortunately there is no resource management for AsyncStream, afaik, there is no interface to disconnect an ongoing AsyncStream. This is why we have rolled out a new version of streaming, which is backed by Reader instead of AsyncStream. We recommend you to switch to the newer version to solve this particular problem. It is very user friendly, you can still keep the AsyncStream but use the Reader.fromAsyncStream interface to create a Reader, then pass the Reader to create a StreamingResponse, and you can use reader.discard() to close streaming connection through the Reader. Here is the user guide: https://docbird.twitter.biz/finatra/user-guide/http/streaming.html. Please let me know if you have any questions!

Now all of the endpoints containing params are not being found.

I'll open a separate ticket as well
Christopher Coco
@cacoco
Thanks it is hard to diagnose your issue when it piggybacked on the other issue which is stating different versions and a somewhat different issue. Thanks!
Also again I am surprised that no params work in your prefixed routes as you can search the tests for these cases...so adding as much context to the issue as possible will be helpful.
Kazuhiro Funakoshi
@kazuf3
Hello. I wonder what's the standard shutdown operations of finatra, because I'm getting NoClassDefFoundError on c/t/util/Promise$ stack trace if I double Ctrl-c in sbt run and this won't happen if I shutdown from admin console on 9990 port. Is this normal behavior? My main object extends HttpServer. Do I have to write something on onExit{} in my class or something?
sinanspd
@sinanspd
You shouldn't need to hook into lifecycle for regular shutdown. HttpServer wraps TwitterServer and manages the basic lifecycle stuff for you. OnExit hook is mostly there if you have modules, databases etc. that need closing/draining before the server shuts down
Kazuhiro Funakoshi
@kazuf3
Thank you, @sinanspd. I don't have any user implemented async process or DB connections. I think I should check my code, starting from build.sbt. What should it look like that minimum build.sbt setup for examples/http-server? The repo's build.sbt seems too big for my simple project...
sinanspd
@sinanspd
The core finatra dependency should be enough to run. Most of the other Twitter dependencies are used in the test scope if I remember correctly. I am omw home, happy to check one of my projects once I get there
Kazuhiro Funakoshi
@kazuf3
Thank you, again. Since I want to follow the test in practice of finatra, I want to also add test dependencies as well, in order to run those test.
Mine is:
name := "inu"

version := "0.1"

scalaVersion := "2.12.8"

libraryDependencies ++= Seq(
  "com.twitter" %% "finatra-http" % "19.10.0",
  "com.twitter" %% "finatra-http" % "19.10.0" % Test classifier "tests",
  "com.twitter" %% "inject-server" % "19.10.0" % Test classifier "tests",
  "com.twitter" %% "inject-app" % "19.10.0" % Test classifier "tests",
  "com.twitter" %% "inject-core" % "19.10.0" % Test classifier "tests",
  "com.twitter" %% "inject-modules" % "19.10.0" % Test classifier "tests",
  "org.scalatest" %% "scalatest" %  "3.0.0" % Test,
  "com.google.inject" % "guice" % "4.2.2",
  "com.google.inject.extensions" % "guice-testlib" % "4.2.2" % Test classifier "tests",
  "ch.qos.logback" % "logback-classic" % "1.1.7",
)
sinanspd
@sinanspd
No problem. Happy to help. build.sbt looks fine to me. Feel free to post your main too. It might be an issue with your jvm set up
jdcohen220
@jdcohen220
Hi there. We currently have our Server extend KafkaStreamsTwitterServer and mix in HttpServer. I have noticed that if any of our kafka streams methods are long-running (even if they aren't blocking) that HTTP request are not being serviced. I have tried wrapping the underlying functionality in a FuturePool but still seeing the same behavior. Is there a way to ensure that HTTP requests are serviced by separate threads than the kafka streams use?
Christopher Coco
@cacoco
FuturePool should ensure this. You are likely orphaning the work, however. If you submit it to a pool then do nothing with the resultant Future, then your main thread continues and can exit before the work you put in the future pool has finished. You generally want to map that future to your main request
sinanspd
@sinanspd
is it wise to use those two together? Both extend twitterserve so imagine some conflict happening?
Kazuhiro Funakoshi
@kazuf3
I have tried in different test on different machine, I found actually there are several patterns of stack traces about Promise, Hotspot and so on when I use C-c C-c in sbt especially after been kept the app up for quite a while (like 3 hours). However, these never come out with admin console shutdown. I decided move forward about my project rather than identifying the cause. I will raise issues once it comes even with admin console shutdown. Thank you for your help.
jdcohen220
@jdcohen220
@cacoco thanks, that makes sense re: possible orphaning the work. How can I ensure they are mapped to the main thread?
Christopher Coco
@cacoco
@jdcohen220 you get a future back when you submit something to a FuturePool, you need to do something with it. I can’t quite tell exactly what you’re trying to do in your case so it’s hard to give great guidance. As @sinanspd pointed out, it’s a bit odd to mix these concerns in this way though I could see you wanting to expose an Http API over some Kakfa work. If it’s the case where the http request is triggering the kafkae work, then it could be as simple as mapping the Future returned from submitting the kafka work to the pool to an HTTP response.
futurepool.submit(work).map(_ => http 201 response)
that would be the return of your http callback in your controller for instance
@kazuf3 running an app from the sbt console will likley always be odd and highly dependent on the sbt configuration you’re using (e.g., are you forking in run, etc). generally we recommend starting your server simply by running the resultant jar. the server can be gracefully shutdown by hitting the /admin/quitquitquit endpoint. killing the process (either manually or through sbt) would not be graceful and all bets are off then as to what errors you may get as it is dependent on what the server is doing when killed.
Christopher Coco
@cacoco
you can take a look at the older examples for info: https://github.com/twitter/finatra/tree/finatra-2.2.0/examples/hello-world
Kazuhiro Funakoshi
@kazuf3

@cacoco Thank you! Yes, sbt-assembly works very well. Also, build options

parallelExecution in ThisBuild := false

fork in Test := true

seems important to get rid of stack traces.

Christopher Coco
@cacoco
@kazuf3 yeah I could see that. for Finatra’s test we also fork in tests and disable parallel execution which is generally a good thing to do when using embedded servers.