You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by janhoy <gi...@git.apache.org> on 2018/09/20 19:59:23 UTC
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
GitHub user janhoy opened a pull request:
https://github.com/apache/lucene-solr/pull/457
SOLR-12791: Add Metrics reporting for AuthenticationPlugin
First partial patch
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cominvent/lucene-solr solr12791-auth-metrics
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/457.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #457
----
commit 89608a9188cca30068f9fb74d5e06054748c6ec1
Author: Jan Høydahl <ja...@...>
Date: 2018-09-20T19:54:12Z
SOLR-12791: First impl. Still no tests
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r224121574
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
--- End diff --
Done
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222587616
--- Diff: solr/core/src/java/org/apache/solr/core/CoreContainer.java ---
@@ -798,6 +798,12 @@ private void reloadSecurityProperties() {
SecurityConfHandler.SecurityConfig securityConfig = securityConfHandler.getSecurityConfig(false);
initializeAuthorizationPlugin((Map<String, Object>) securityConfig.getData().get("authorization"));
initializeAuthenticationPlugin((Map<String, Object>) securityConfig.getData().get("authentication"));
+ if (authenticationPlugin != null && authenticationPlugin.plugin.getMetricRegistry() == null) {
--- End diff --
Is this second check necessary? we know that just after the plugin was created its metricRegistry is null, it's set only after `initializeMetrics` has been called.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222759868
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
--- End diff --
No plugins currently log timeouts. I think it should perhaps be removed and instead re-introduced if we see that some auth plugins with external network depenedencies feel the need for logging number of (external) timeouts.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r223288246
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
--- End diff --
I think that malformed credentials should be simply reported as errors.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r223290128
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
+ numInvalidCredentials = manager.counter(this, registryName, "failInvalidCredentials", getCategory().toString(), scope);
+ numMissingCredentials = manager.counter(this, registryName, "failMissingCredentials", getCategory().toString(), scope);
+ requestTimes = manager.timer(this, registryName, "requestTimes", getCategory().toString(), scope);
+ totalTime = manager.counter(this, registryName, "totalTime", getCategory().toString(), scope);
+ metricNames.addAll(Arrays.asList("errors", "timeouts", "requests", "authenticated", "passThrough",
+ "failWrongCredentials", "failMissingCredentials", "failInvalidCredentials", "requestTimes", "totalTime"));
+ }
+
+ @Override
+ public String getName() {
+ return this.getClass().getName();
+ }
+
+ @Override
+ public String getDescription() {
+ return this.getClass().getName();
--- End diff --
Perhaps it should describe at least what the base class' function is, in this case `"Authentication Plugin " + this.getClass().getSimpleName()`. This way subclasses would automatically report the base function and their specific class name (or they could override it if needed).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222761691
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
+ numInvalidCredentials = manager.counter(this, registryName, "failInvalidCredentials", getCategory().toString(), scope);
+ numMissingCredentials = manager.counter(this, registryName, "failMissingCredentials", getCategory().toString(), scope);
+ requestTimes = manager.timer(this, registryName, "requestTimes", getCategory().toString(), scope);
+ totalTime = manager.counter(this, registryName, "totalTime", getCategory().toString(), scope);
+ metricNames.addAll(Arrays.asList("errors", "timeouts", "requests", "authenticated", "passThrough",
+ "failWrongCredentials", "failMissingCredentials", "failInvalidCredentials", "requestTimes", "totalTime"));
+ }
+
+ @Override
+ public String getName() {
+ return this.getClass().getName();
+ }
+
+ @Override
+ public String getDescription() {
+ return this.getClass().getName();
--- End diff --
I did not want to extend the API surface of the Auth plugins to require a description, Any suggestions of how to obtain better description from somewhere else?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222593938
--- Diff: solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java ---
@@ -420,4 +425,68 @@ public static void ensureRunningJettys(int nodeCount, int timeoutSeconds) throws
cluster.waitForAllNodes(timeoutSeconds);
}
+
+ static final List<String> AUTH_METRICS_KEYS = Arrays.asList("errors", "timeouts", "requests", "authenticated",
--- End diff --
I think it would be better to put this whole section in a subclass of SolrCloudTestCase, eg. SolrCloudAuthTestCase - there are only two tests that need this stuff but there are hundreds of tests that extend this base class and don't need this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r223286109
--- Diff: solr/core/src/java/org/apache/solr/core/SolrInfoBean.java ---
@@ -34,7 +34,7 @@
* Category of Solr component.
*/
enum Category { CONTAINER, ADMIN, CORE, QUERY, UPDATE, CACHE, HIGHLIGHTER, QUERYPARSER, SPELLCHECKER,
- SEARCHER, REPLICATION, TLOG, INDEX, DIRECTORY, HTTP, OTHER }
+ SEARCHER, REPLICATION, TLOG, INDEX, DIRECTORY, HTTP, SECURITY, OTHER }
--- End diff --
SECURITY makes more sense imho.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r224121448
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
+ numInvalidCredentials = manager.counter(this, registryName, "failInvalidCredentials", getCategory().toString(), scope);
+ numMissingCredentials = manager.counter(this, registryName, "failMissingCredentials", getCategory().toString(), scope);
+ requestTimes = manager.timer(this, registryName, "requestTimes", getCategory().toString(), scope);
+ totalTime = manager.counter(this, registryName, "totalTime", getCategory().toString(), scope);
+ metricNames.addAll(Arrays.asList("errors", "timeouts", "requests", "authenticated", "passThrough",
+ "failWrongCredentials", "failMissingCredentials", "failInvalidCredentials", "requestTimes", "totalTime"));
+ }
+
+ @Override
+ public String getName() {
+ return this.getClass().getName();
+ }
+
+ @Override
+ public String getDescription() {
+ return this.getClass().getName();
--- End diff --
Done
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222657709
--- Diff: solr/core/src/java/org/apache/solr/core/CoreContainer.java ---
@@ -798,6 +798,12 @@ private void reloadSecurityProperties() {
SecurityConfHandler.SecurityConfig securityConfig = securityConfHandler.getSecurityConfig(false);
initializeAuthorizationPlugin((Map<String, Object>) securityConfig.getData().get("authorization"));
initializeAuthenticationPlugin((Map<String, Object>) securityConfig.getData().get("authentication"));
+ if (authenticationPlugin != null && authenticationPlugin.plugin.getMetricRegistry() == null) {
--- End diff --
Yea, I think you're right.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222757598
--- Diff: solr/core/src/java/org/apache/solr/core/SolrInfoBean.java ---
@@ -34,7 +34,7 @@
* Category of Solr component.
*/
enum Category { CONTAINER, ADMIN, CORE, QUERY, UPDATE, CACHE, HIGHLIGHTER, QUERYPARSER, SPELLCHECKER,
- SEARCHER, REPLICATION, TLOG, INDEX, DIRECTORY, HTTP, OTHER }
+ SEARCHER, REPLICATION, TLOG, INDEX, DIRECTORY, HTTP, SECURITY, OTHER }
--- End diff --
I chose a new category because over time I'd like to add metrics also for Authorization plugins and Auditlog plugins (all components registered in security.json). An alternative could have been `CONTAINER` I guess?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222759422
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
--- End diff --
Is this re-throw safe?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222761151
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
+ requests = manager.counter(this, registryName, "requests", getCategory().toString(), scope);
+ numAuthenticated = manager.counter(this, registryName, "authenticated", getCategory().toString(), scope);
+ numPassThrough = manager.counter(this, registryName, "passThrough", getCategory().toString(), scope);
+ numWrongCredentials = manager.counter(this, registryName, "failWrongCredentials", getCategory().toString(), scope);
--- End diff --
If auth does not succeed we log either wrong credentials, invalid credentials (e.g. malformed, if basicAuth header does not contain a ':'), missing credentials (if no header at all). Not all plugins will be able to distinguish between all these, such as hadoop plugin which I raised a question about in the JIRA. Should we have a more coarse-grained metric on this? Perhaps these are enough: `numAuthenticated`, `numPassThrough`, `numFailedAuth` and `numErrors`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222759139
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -20,16 +20,44 @@
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import java.io.Closeable;
+import java.util.Arrays;
import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Timer;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.apache.solr.metrics.SolrMetricProducer;
/**
*
* @lucene.experimental
*/
-public abstract class AuthenticationPlugin implements Closeable {
+public abstract class AuthenticationPlugin implements Closeable, SolrInfoBean, SolrMetricProducer {
--- End diff --
I chose to add the `SolrMetricsProducer` on the base class instead of letting each Auth plugin choose to implement it or not. The effect will be that plugins that do not change their code to increment counters will be logged as 0 for these counters. But the `requests` count, `requestTimes` stats, `totalTime` and `errors` will still be logged by the base class on behalf of all plugins even if they don't increment the other counters.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r222658500
--- Diff: solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java ---
@@ -420,4 +425,68 @@ public static void ensureRunningJettys(int nodeCount, int timeoutSeconds) throws
cluster.waitForAllNodes(timeoutSeconds);
}
+
+ static final List<String> AUTH_METRICS_KEYS = Arrays.asList("errors", "timeouts", "requests", "authenticated",
--- End diff --
Yep, a separate test base class for auth tests makes sense. Good idea. We could then push down some other utility methods from the tests as well, I see some duplication a few places.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by janhoy <gi...@git.apache.org>.
Github user janhoy closed the pull request at:
https://github.com/apache/lucene-solr/pull/457
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #457: SOLR-12791: Add Metrics reporting for Authent...
Posted by sigram <gi...@git.apache.org>.
Github user sigram commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/457#discussion_r223286527
--- Diff: solr/core/src/java/org/apache/solr/security/AuthenticationPlugin.java ---
@@ -52,11 +80,73 @@
public abstract boolean doAuthenticate(ServletRequest request, ServletResponse response,
FilterChain filterChain) throws Exception;
+ /**
+ * This method is called by SolrDispatchFilter in order to initiate authentication.
+ * It does some standard metrics counting.
+ */
+ public final boolean authenticate(ServletRequest request, ServletResponse response, FilterChain filterChain) throws Exception {
+ Timer.Context timer = requestTimes.time();
+ requests.inc();
+ try {
+ return doAuthenticate(request, response, filterChain);
+ } catch(Exception e) {
+ numErrors.mark();
+ throw e;
+ } finally {
+ long elapsed = timer.stop();
+ totalTime.inc(elapsed);
+ }
+ }
/**
* Cleanup any per request data
*/
public void closeRequest() {
}
+ @Override
+ public void initializeMetrics(SolrMetricManager manager, String registryName, String tag, final String scope) {
+ this.metricManager = manager;
+ this.registryName = registryName;
+ // Metrics
+ registry = manager.registry(registryName);
+ numErrors = manager.meter(this, registryName, "errors", getCategory().toString(), scope);
+ numTimeouts = manager.meter(this, registryName, "timeouts", getCategory().toString(), scope);
--- End diff --
+1 to remove and add it later as needed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org