OK. I have reverted the changes. Martin Grigorov Wicket Training and Consulting https://twitter.com/mtgrigorov
On Mon, Mar 28, 2016 at 3:17 PM, Tobias Soloschenko < [email protected]> wrote: > I am not happy with those changes. Because of the following points: > > 1. You only changed the signature of the count method in WicketMetrics > which is used for counting sessions in the SessionCountListenerAspect - the > other methods mark / measureTime are also using the Settings and Registry > and do not access it the same way the count method does > > 2. This will lead to the situation that some Aspects are receiving a > different Settings / Registry then the SessionCount does if they are called > before the listener is started when a session is restored > > 3. The other method signatures can't be changed this way, because they > might not have any options to access the ServletContent via Aspect. (e.g. > new operations) > > I would prefer to stay with the old signature and find a way to handle > with different applications in the WicketMetrics class. I would be > perfectly fine if we remove the statics, but we should not change signature > of the convenience methods that should be nearly the same (mark, > measureTime, count) > > kind regards > > Tobias > > > Am 28.03.16 um 14:50 schrieb Martin Grigorov: > > That's the reason to skip calling #inc() and #dec() if there is no >> MetricRegistry in the ServletContext. >> >> Static is evil ;-) >> Try to avoid it as much as possible. >> >> Despite being Apache Tomcat committer my only application that uses Wicket >> 8.0.0-SNAPSHOT runs on Jetty :-) >> I'm too lazy to setup development environment with Tomcat for now. >> >> On Mon, Mar 28, 2016 at 2:43 PM, Tobias Soloschenko < >> [email protected]> wrote: >> >> Have you tested the startup order of the classes? AOP is started before >>> the webapp starts and with it the listener. So if tomcat restore a >>> session >>> - the new constructor of some components can be invoked and then the >>> setup >>> fails because there might be aspects around it. >>> >>> So check if the listener is started before any getSettings() or >>> getRegistry() is invoked. Even if sessions are restored after server >>> start. >>> >>> That was the original reason why I used statics. >>> >>> kind regards >>> >>> Tobias >>> >>> Am 28.03.2016 um 14:00 schrieb Martin Grigorov <[email protected]>: >>>> >>>> I am not sure what you mean. >>>> >>>> SessionCountListener uses @WebListener so it will be automatically added >>>> >>> to >>> >>>> the application if the metadata is not complete. >>>> If <web-app metadata-complete="true"> then the application developer >>>> will >>>> have to add a <listener> manually if (s)he wants to use it. >>>> >>>> SessionCountListener#inc() and #dec() methods will be called only if >>>> >>> there >>> >>>> is MetricRegistry in the ServletContext, i.e. only if wicket-metrics is >>>> >>> in >>> >>>> the classpath because org.apache.wicket.metrics.Initializer takes care >>>> to >>>> set it up. >>>> >>>> As I said in my previous mail the static and #get(String) are not needed >>>> >>> by >>> >>>> any of the current aspects, so I think it is OK to remove them. We can >>>> reintroduce them if someone comes with a use case that is not supported >>>> >>> by >>> >>>> the threadlocal and/or the servlet context. >>>> >>>> Martin Grigorov >>>> Wicket Training and Consulting >>>> https://twitter.com/mtgrigorov >>>> >>>> On Mon, Mar 28, 2016 at 12:51 PM, Tobias Soloschenko < >>>> [email protected]> wrote: >>>> >>>> So the Listener is required, now? Or is this only an option if you have >>>>> more then one application in one war? >>>>> >>>>> I created the SessionCountListener only as a helper for the aspect to >>>>> count sessions. Now it is used to store the session / the registry. >>>>> >>>>> About the statics and get(String)? Can they he removed? >>>>> >>>>> kind regards >>>>> >>>>> Tobias >>>>> >>>>> Am 28.03.2016 um 12:40 schrieb Martin Grigorov <[email protected]>: >>>>>> >>>>>> I've pushed the second part. >>>>>> Now SessionCountListener and SessionCountListenerAspect use the >>>>>> MetricRegistry and WicketMetricsSettings stored in the ServletContext. >>>>>> >>>>>> For applications which live in the same .war file the registry and the >>>>>> settings are shared by default. Each application can override any of >>>>>> >>>>> those >>>>> >>>>>> in MyApp#init() method >>>>>> Applications in separate .war files will have different registry and >>>>>> settings. >>>>>> >>>>>> I don't see a need to use <env-entry> for any use case now. >>>>>> The other ways to lookup the registry and the settings could stay, >>>>>> >>>>> although >>>>> >>>>>> all current aspects could work by using the ThreadLocal or the >>>>>> ServletContext. >>>>>> >>>>>> Martin Grigorov >>>>>> Wicket Training and Consulting >>>>>> https://twitter.com/mtgrigorov >>>>>> >>>>>> On Mon, Mar 28, 2016 at 10:10 AM, Tobias Soloschenko < >>>>>> [email protected]> wrote: >>>>>> >>>>>> Mhhh the SessionCountListener is optional - keep that in mind - if a >>>>>>> Session is restored this might cause issues but I am looking forward >>>>>>> >>>>>> to >>> >>>> see >>>>> >>>>>> the final changes. >>>>>>> >>>>>>> kind regards >>>>>>> >>>>>>> Tobias >>>>>>> >>>>>>> Am 28.03.2016 um 09:28 schrieb Martin Grigorov <[email protected] >>>>>>>> >>>>>>> : >>>> >>>>> I've pushed a commit that I believe will help. >>>>>>>> I have to go out now, so I'll finish it in the afternoon. >>>>>>>> >>>>>>>> Martin Grigorov >>>>>>>> Wicket Training and Consulting >>>>>>>> https://twitter.com/mtgrigorov >>>>>>>> >>>>>>>> On Mon, Mar 28, 2016 at 9:25 AM, Tobias Soloschenko < >>>>>>>> [email protected]> wrote: >>>>>>>> >>>>>>>> Hi Martin, >>>>>>>>> >>>>>>>>> thank you! >>>>>>>>> >>>>>>>>> kind regards >>>>>>>>> >>>>>>>>> Tobias >>>>>>>>> >>>>>>>>> Am 28.03.2016 um 09:15 schrieb Martin Grigorov < >>>>>>>>>> >>>>>>>>> [email protected] >>> >>>> : >>>>>> >>>>>>> Hi Tobias, >>>>>>>>>> >>>>>>>>>> I haven't tested it with two .war files yet but I can see how to >>>>>>>>>> >>>>>>>>> break >>>>> >>>>>> this >>>>>>>>> >>>>>>>>>> approach too: add two Wicket applications in the same .war, i.e. >>>>>>>>>> >>>>>>>>> two >>> >>>> <filter>s for two different applications. >>>>>>>>>> The env-entry is shared for all servlets/filters in the .war. >>>>>>>>>> >>>>>>>>>> I'll try to rework it to use the ServletContext now. >>>>>>>>>> >>>>>>>>>> Martin Grigorov >>>>>>>>>> Wicket Training and Consulting >>>>>>>>>> https://twitter.com/mtgrigorov >>>>>>>>>> >>>>>>>>>> On Mon, Mar 28, 2016 at 7:51 AM, Tobias Soloschenko < >>>>>>>>>> [email protected]> wrote: >>>>>>>>>> >>>>>>>>>> Update: >>>>>>>>>>> >>>>>>>>>>> I finally got it working for me: >>>>>>>>>>> >>>>>>>>>> >>> https://github.com/klopfdreh/wicket/commit/dda71c9e5ce5135993df0ea85450d232f14e53c5 >>> >>>> @Martin: Would be able to also test it with two or more >>>>>>>>>>> >>>>>>>>>> applications >>> >>>> running on the same server? >>>>>>>>>>> >>>>>>>>>>> In your MetricsServlet.ContextListener you could easily write >>>>>>>>>>> WicketMetrics.getMetricsRegistry() - The detection which >>>>>>>>>>> >>>>>>>>>> application >>> >>>> should >>>>>>>>> >>>>>>>>>> be shipped is handled if you set up environment entries in each >>>>>>>>>>> >>>>>>>>>> project. >>>>>>> >>>>>>>> (see commit) >>>>>>>>>>> >>>>>>>>>> >>> https://dropwizard.github.io/metrics/3.1.0/manual/servlets/#manual-servlets >>> >>>> kind regards >>>>>>>>>>> >>>>>>>>>>> Tobias >>>>>>>>>>> >>>>>>>>>>> Am 27.03.16 um 13:12 schrieb Martin Grigorov: >>>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>>> The problem with -Dwicket.metrics.applicationName and the static >>>>>>>>>>>> >>>>>>>>>>> variables >>>>>>>>> >>>>>>>>>> approach is that both cannot be used if you have two Wicket >>>>>>>>>>>> >>>>>>>>>>> applications >>>>>>>>> >>>>>>>>>> in >>>>>>>>>>>> the web server. >>>>>>>>>>>> >>>>>>>>>>>> There is no need to implement a custom Servlet. metrics-servlet >>>>>>>>>>>> >>>>>>>>>>> already >>>>>>> >>>>>>>> provides it. We just have to make the MetricsRegistry available >>>>>>>>>>>> >>>>>>>>>>> in >>> >>>> the >>>>>>> >>>>>>>> ServletContext (attribute >>>>>>>>>>>> "com.codahale.metrics.servlets.MetricsServlet.registry") >>>>>>>>>>>> >>>>>>>>>>>> Martin Grigorov >>>>>>>>>>>> Wicket Training and Consulting >>>>>>>>>>>> https://twitter.com/mtgrigorov >>>>>>>>>>>> >>>>>>>>>>>> On Sat, Mar 26, 2016 at 2:34 AM, Tobias Soloschenko < >>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>>> I would prefer to stay by the Application.get(String) / >>>>>>>>>>>>> >>>>>>>>>>>> Application.get() >>>>>>>>> >>>>>>>>>> / static because it is more Independent and not bound to the >>>>>>>>>>>>> >>>>>>>>>>>> webapp >>>>> >>>>>> lifecycle. Currently we only rely on the servlet stuff in one >>>>>>>>>>>>> >>>>>>>>>>>> metric - >>>>>>> >>>>>>>> which is at least not required. Even in Wicket itself you should >>>>>>>>>>>>> >>>>>>>>>>>> not >>>>> >>>>>> access >>>>>>>>>>>>> the HttpSession itself. >>>>>>>>>>>>> >>>>>>>>>>>>> But this is only my opinion - let us hear other suggestions. >>>>>>>>>>>>> >>>>>>>>>>>>> Maybe it is a good idea to implement a defaul servlet which can >>>>>>>>>>>>> >>>>>>>>>>>> be >>> >>>> configured and exposes the metrics / registry via http get - >>>>>>>>>>>>> >>>>>>>>>>>> which >>> >>>> uses >>>>>>>>> >>>>>>>>>> the >>>>>>>>>>>>> dropwizard metric servlet. >>>>>>>>>>>>> >>>>>>>>>>>>> kind regards >>>>>>>>>>>>> >>>>>>>>>>>>> Tobias >>>>>>>>>>>>> >>>>>>>>>>>>> Am 25.03.2016 um 22:51 schrieb Martin Grigorov < >>>>>>>>>>>>>> >>>>>>>>>>>>> [email protected] >>>>>>> >>>>>>>> : >>>>>>>>>> >>>>>>>>>>> Just moments after sending the mail I recalled that DropWizard >>>>>>>>>>>>>> >>>>>>>>>>>>> provides >>>>>>>>> >>>>>>>>>> something similar: >>>>>>>>>>>>>> https://dropwizard.github.io/metrics/3.1.0/manual/servlets/ >>>>>>>>>>>>>> So there is no need of a custom IResource. >>>>>>>>>>>>>> We just have to make it easier to lookup the MetricsRegistry >>>>>>>>>>>>>> >>>>>>>>>>>>> from >>> >>>> MetricsServlet - via the ServletContext. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I wonder whether the ServletContext solution could be used >>>>>>>>>>>>>> >>>>>>>>>>>>> instead >>>>> >>>>>> of >>>>>>> >>>>>>>> the >>>>>>>>>>>>>> Application#get(String) and static variable fallbacks. I.e. >>>>>>>>>>>>>> >>>>>>>>>>>>> somehow >>>>> >>>>>> to >>>>>>>>> >>>>>>>>>> get >>>>>>>>>>>>> >>>>>>>>>>>>> a reference to ServletContext in Session#onInvalidate(). >>>>>>>>>>>>>> >>>>>>>>>>>>>> Martin Grigorov >>>>>>>>>>>>>> Wicket Training and Consulting >>>>>>>>>>>>>> https://twitter.com/mtgrigorov >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Fri, Mar 25, 2016 at 10:48 PM, Martin Grigorov < >>>>>>>>>>>>>> >>>>>>>>>>>>> [email protected] >>>>>>>>> >>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Tobias, >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Inspired by >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>> https://github.com/jooby-project/jooby/tree/master/jooby-metrics >>>>> >>>>>> I >>>>>>> >>>>>>>> think >>>>>>>>>>>>>> it would be nice if wicket-metrics provides a IResource that >>>>>>>>>>>>>> >>>>>>>>>>>>> renders >>>>>>> >>>>>>>> JSON >>>>>>>>>>>>>> with the current metrics per type/aspect. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> I.e. if /wicket/metrics/ is requested then it dumps something >>>>>>>>>>>>>>> >>>>>>>>>>>>>> like: >>>>>>> >>>>>>>> { >>>>>>>>>>>>>>> "SomeTimerAspect" : {min:.., max:..., mean:..., ...}, >>>>>>>>>>>>>>> ... >>>>>>>>>>>>>>> "SomeCounterAspect" : {value:..}, >>>>>>>>>>>>>>> ... >>>>>>>>>>>>>>> } >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> When /wicket/metrics/SomeCounterAspect is requested then : >>>>>>>>>>>>>>> {"value": ...} >>>>>>>>>>>>>>> is rendered. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Do you think it is a good idea ? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> It will be useful for quicker checks of the current state. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The application will have to mount it explicitly in >>>>>>>>>>>>>>> >>>>>>>>>>>>>> MyApp#init(). >>>>> >>>>>> >>>>>>>>>>>>>>> Martin Grigorov >>>>>>>>>>>>>>> Wicket Training and Consulting >>>>>>>>>>>>>>> https://twitter.com/mtgrigorov >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >
