Skip to content

Commit 8fee442

Browse files
tulinkryVladimir Kotal
tulinkry
authored and
Vladimir Kotal
committed
adding per stack statistics for authorization (#1653)
fixes #1568
1 parent ff21eb8 commit 8fee442

File tree

9 files changed

+67
-44
lines changed

9 files changed

+67
-44
lines changed

‎src/org/opensolaris/opengrok/authorization/AuthorizationEntity.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@ public boolean test(IAuthorizationPlugin t) {
8585
* @return true if plugin allows the action; false otherwise
8686
*/
8787
public abstract boolean decision(IAuthorizationPlugin t);
88-
8988
}
9089

9190
/**

‎src/org/opensolaris/opengrok/authorization/AuthorizationFramework.java

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,10 @@ public boolean isAllowed(HttpServletRequest request, Project project) {
156156
"plugin_framework_project_cache",
157157
project,
158158
new AuthorizationEntity.PluginDecisionPredicate() {
159-
@Override
160-
public boolean decision(IAuthorizationPlugin plugin) {
161-
return plugin.isAllowed(request, project);
162-
}
159+
@Override
160+
public boolean decision(IAuthorizationPlugin plugin) {
161+
return plugin.isAllowed(request, project);
162+
}
163163
}, new AuthorizationEntity.PluginSkippingPredicate() {
164164
@Override
165165
public boolean shouldSkip(AuthorizationEntity authEntity) {
@@ -194,10 +194,10 @@ public boolean isAllowed(HttpServletRequest request, Group group) {
194194
"plugin_framework_group_cache",
195195
group,
196196
new AuthorizationEntity.PluginDecisionPredicate() {
197-
@Override
198-
public boolean decision(IAuthorizationPlugin plugin) {
199-
return plugin.isAllowed(request, group);
200-
}
197+
@Override
198+
public boolean decision(IAuthorizationPlugin plugin) {
199+
return plugin.isAllowed(request, group);
200+
}
201201
}, new AuthorizationEntity.PluginSkippingPredicate() {
202202
@Override
203203
public boolean shouldSkip(AuthorizationEntity authEntity) {
@@ -667,26 +667,26 @@ private boolean checkAll(HttpServletRequest request, String cache, Nameable enti
667667
m = new TreeMap<>();
668668
} else if ((val = m.get(entity.getName())) != null) {
669669
// cache hit
670-
stats.addRequest(request, "authorization_cache_hits");
670+
stats.addRequest("authorization_cache_hits");
671671
return val;
672672
}
673673

674-
stats.addRequest(request, "authorization_cache_misses");
674+
stats.addRequest("authorization_cache_misses");
675675

676676
long time = System.currentTimeMillis();
677677

678678
boolean overallDecision = performCheck(entity, pluginPredicate, skippingPredicate);
679679

680680
time = System.currentTimeMillis() - time;
681681

682-
stats.addRequestTime(request, "authorization", time);
683-
stats.addRequestTime(request,
682+
stats.addRequestTime("authorization", time);
683+
stats.addRequestTime(
684684
String.format("authorization_%s", overallDecision ? "positive" : "negative"),
685685
time);
686-
stats.addRequestTime(request,
686+
stats.addRequestTime(
687687
String.format("authorization_%s_of_%s", overallDecision ? "positive" : "negative", entity.getName()),
688688
time);
689-
stats.addRequestTime(request,
689+
stats.addRequestTime(
690690
String.format("authorization_of_%s", entity.getName()),
691691
time);
692692

‎src/org/opensolaris/opengrok/authorization/AuthorizationStack.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,9 @@
2929
import java.util.logging.Level;
3030
import java.util.logging.Logger;
3131
import org.opensolaris.opengrok.configuration.Nameable;
32+
import org.opensolaris.opengrok.configuration.RuntimeEnvironment;
3233
import org.opensolaris.opengrok.logger.LoggerFactory;
34+
import org.opensolaris.opengrok.web.Statistics;
3335

3436
/**
3537
* This is a subclass of {@link AuthorizationEntity} implementing the methods to
@@ -216,6 +218,10 @@ public boolean isAllowed(Nameable entity,
216218
protected boolean processStack(Nameable entity,
217219
PluginDecisionPredicate pluginPredicate,
218220
PluginSkippingPredicate skippingPredicate) {
221+
222+
Statistics stats = RuntimeEnvironment.getInstance().getStatistics();
223+
long time = System.currentTimeMillis();
224+
219225
boolean overallDecision = true;
220226
for (AuthorizationEntity authEntity : getStack()) {
221227

@@ -269,6 +275,18 @@ protected boolean processStack(Nameable entity,
269275
}
270276
}
271277
}
278+
time = System.currentTimeMillis() - time;
279+
280+
stats.addRequestTime(
281+
String.format("authorization_in_stack_%s_%s", getName(), overallDecision ? "positive" : "negative"),
282+
time);
283+
stats.addRequestTime(
284+
String.format("authorization_in_stack_%s_%s_of_%s", getName(), overallDecision ? "positive" : "negative", entity.getName()),
285+
time);
286+
stats.addRequestTime(
287+
String.format("authorization_in_stack_%s_of_%s", getName(), entity.getName()),
288+
time);
289+
272290
return overallDecision;
273291
}
274292

‎src/org/opensolaris/opengrok/web/AuthorizationFilter.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,17 @@ public void doFilter(ServletRequest sr, ServletResponse sr1, FilterChain fc) thr
6161
} else {
6262
LOGGER.log(Level.INFO, "Access denied for URI: {0}", httpReq.getRequestURI());
6363
}
64-
config.getEnv().getStatistics().addRequest(httpReq);
65-
config.getEnv().getStatistics().addRequest(httpReq, "requests_forbidden");
66-
config.getEnv().getStatistics().addRequestTime(httpReq,
67-
"requests_forbidden",
64+
/**
65+
* Add the request to the statistics. This is called just once for a
66+
* single request otherwise the next filter will count the same
67+
* request twice ({@link StatisticsFilter#collectStats}).
68+
*
69+
* In this branch of the if statement the filter processing stopped
70+
* and does not follow to the StatisticsFilter.
71+
*/
72+
config.getEnv().getStatistics().addRequest();
73+
config.getEnv().getStatistics().addRequest("requests_forbidden");
74+
config.getEnv().getStatistics().addRequestTime("requests_forbidden",
6875
System.currentTimeMillis() - processTime);
6976
if (!config.getEnv().getConfiguration().getForbiddenIncludeFileContent().isEmpty()) {
7077
sr.getRequestDispatcher("/eforbidden").forward(sr, sr1);

‎src/org/opensolaris/opengrok/web/Statistics.java

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919

2020
/*
21-
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
21+
* Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
2222
*/
2323
package org.opensolaris.opengrok.web;
2424

@@ -27,7 +27,6 @@
2727
import java.util.TreeMap;
2828
import java.util.stream.Collectors;
2929
import java.util.stream.LongStream;
30-
import javax.servlet.http.HttpServletRequest;
3130
import org.json.simple.JSONArray;
3231
import org.json.simple.JSONObject;
3332

@@ -67,7 +66,7 @@ public class Statistics {
6766
/**
6867
* Adds a single request into all requests.
6968
*/
70-
synchronized public void addRequest(HttpServletRequest req) {
69+
synchronized public void addRequest() {
7170
maybeRefresh();
7271

7372
requestsPerMinute++;
@@ -99,10 +98,9 @@ synchronized protected void maybeRefresh() {
9998
/**
10099
* Adds a request into the category
101100
*
102-
* @param req the given request
103101
* @param category category
104102
*/
105-
synchronized public void addRequest(HttpServletRequest req, String category) {
103+
synchronized public void addRequest(String category) {
106104
Long val = requestCategories.get(category);
107105
if (val == null) {
108106
val = new Long(0);
@@ -114,12 +112,11 @@ synchronized public void addRequest(HttpServletRequest req, String category) {
114112
/**
115113
* Adds a request's process time into given category.
116114
*
117-
* @param req the given request
118115
* @param category category
119116
* @param v time spent on processing this request
120117
*/
121-
synchronized public void addRequestTime(HttpServletRequest req, String category, long v) {
122-
addRequest(req, category);
118+
synchronized public void addRequestTime(String category, long v) {
119+
addRequest(category);
123120
Long val = timing.get(category);
124121
Long min = timingMin.get(category);
125122
Long max = timingMax.get(category);

‎src/org/opensolaris/opengrok/web/StatisticsFilter.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919

2020
/*
21-
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
21+
* Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
2222
*/
2323
package org.opensolaris.opengrok.web;
2424

@@ -69,27 +69,31 @@ protected void collectStats(HttpServletRequest req, String category) {
6969
PageConfig config = PageConfig.get(req);
7070
Statistics stats = config.getEnv().getStatistics();
7171

72-
stats.addRequest(req);
72+
/**
73+
* Add the request to the statistics. Be aware of the colliding call in
74+
* {@code AuthorizationFilter#doFilter}.
75+
*/
76+
stats.addRequest();
7377

7478
if ((o = config.getRequestAttribute(TIME_ATTRIBUTE)) != null) {
7579
processTime = System.currentTimeMillis() - (long) o;
7680

77-
stats.addRequestTime(req, "*", processTime); // add to all
78-
stats.addRequestTime(req, category, processTime); // add this category
81+
stats.addRequestTime("*", processTime); // add to all
82+
stats.addRequestTime(category, processTime); // add this category
7983

8084
/* supplementary categories */
8185
if (config.getProject() != null) {
82-
stats.addRequestTime(req, "viewing_of_" + config.getProject().getName(), processTime);
86+
stats.addRequestTime("viewing_of_" + config.getProject().getName(), processTime);
8387
}
8488

8589
SearchHelper helper = (SearchHelper) config.getRequestAttribute(SearchHelper.REQUEST_ATTR);
8690
if (helper != null) {
8791
if (helper.hits == null || helper.hits.length == 0) {
8892
// empty search
89-
stats.addRequestTime(req, "empty_search", processTime);
93+
stats.addRequestTime("empty_search", processTime);
9094
} else {
9195
// successful search
92-
stats.addRequestTime(req, "successful_search", processTime);
96+
stats.addRequestTime("successful_search", processTime);
9397
}
9498
}
9599
}

‎test/org/opensolaris/opengrok/configuration/RuntimeEnvironmentTest.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@
5050
import org.opensolaris.opengrok.configuration.messages.Message;
5151
import org.opensolaris.opengrok.configuration.messages.NormalMessage;
5252
import org.opensolaris.opengrok.history.RepositoryInfo;
53-
import org.opensolaris.opengrok.web.DummyHttpServletRequest;
5453
import org.opensolaris.opengrok.web.Statistics;
5554

5655
import static org.junit.Assert.assertEquals;
@@ -1026,9 +1025,9 @@ public void testSaveEmptyStatistics() throws IOException {
10261025
public void testSaveStatistics() throws IOException {
10271026
RuntimeEnvironment env = RuntimeEnvironment.getInstance();
10281027
env.setStatistics(new Statistics());
1029-
env.getStatistics().addRequest(new DummyHttpServletRequest());
1030-
env.getStatistics().addRequest(new DummyHttpServletRequest(), "root");
1031-
env.getStatistics().addRequestTime(new DummyHttpServletRequest(), "root", 10L);
1028+
env.getStatistics().addRequest();
1029+
env.getStatistics().addRequest("root");
1030+
env.getStatistics().addRequestTime("root", 10L);
10321031

10331032
try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
10341033
env.saveStatistics(out);

‎test/org/opensolaris/opengrok/configuration/messages/StatsMessageTest.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
import org.junit.Before;
3232
import org.junit.Test;
3333
import org.opensolaris.opengrok.configuration.RuntimeEnvironment;
34-
import org.opensolaris.opengrok.web.DummyHttpServletRequest;
3534
import org.opensolaris.opengrok.web.Statistics;
3635

3736
/**
@@ -105,7 +104,7 @@ public void testGetClean() {
105104
@Test
106105
public void testGet() {
107106
testClean();
108-
env.getStatistics().addRequest(new DummyHttpServletRequest());
107+
env.getStatistics().addRequest();
109108
Message m = new StatsMessage();
110109
m.setText("get");
111110
byte[] out = null;

‎test/org/opensolaris/opengrok/web/StatisticsTest.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ public void testAddRequestTimeCategory() {
101101
};
102102
for (String category : testCategories) {
103103
Statistics stat = new Statistics();
104-
stat.addRequestTime(new DummyHttpServletRequest(), category, (long) (Math.random() * Long.MAX_VALUE));
104+
stat.addRequestTime(category, (long) (Math.random() * Long.MAX_VALUE));
105105
Assert.assertEquals(1, stat.getRequestCategories().size());
106106
Assert.assertTrue(stat.getRequestCategories().containsKey(category));
107107
Assert.assertNotNull(stat.getRequestCategories().get(category));
@@ -139,7 +139,7 @@ public void testAddRequestTimeMultipleCategories() {
139139
Assert.assertEquals(testCategories.length, testSizes.length);
140140

141141
for (int i = 0; i < testCategories.length; i++) {
142-
stat.addRequestTime(new DummyHttpServletRequest(), testCategories[i], (long) (Math.random() * Long.MAX_VALUE));
142+
stat.addRequestTime(testCategories[i], (long) (Math.random() * Long.MAX_VALUE));
143143
Assert.assertEquals(testSizes[i], stat.getRequestCategories().size());
144144
Assert.assertTrue(stat.getRequestCategories().containsKey(testCategories[i]));
145145
Assert.assertNotNull(stat.getRequestCategories().get(testCategories[i]));
@@ -172,7 +172,7 @@ public void testAddRequestTimeSum() {
172172

173173
for (int i = 0; i < testCategories.length; i++) {
174174
Statistics stat = new Statistics();
175-
stat.addRequestTime(new DummyHttpServletRequest(), testCategories[i], testValues[i]);
175+
stat.addRequestTime(testCategories[i], testValues[i]);
176176
Assert.assertEquals(testValues[i], stat.getTiming().get(testCategories[i]).longValue());
177177
}
178178
}
@@ -194,7 +194,7 @@ public void testAddRequestMultipleTimeSum() {
194194
Assert.assertEquals(testCategories.length, testExpected.length);
195195

196196
for (int i = 0; i < testCategories.length; i++) {
197-
stat.addRequestTime(new DummyHttpServletRequest(), testCategories[i], testValues[i]);
197+
stat.addRequestTime(testCategories[i], testValues[i]);
198198
Assert.assertEquals(testExpected[i], stat.getTiming().get(testCategories[i]).longValue());
199199
}
200200
}

0 commit comments

Comments
 (0)