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
