Skip to content

Commit acd44da

Browse files
justinstollerjustinstoller
authored
Merge pull request #2880 from justinstoller/PE-38408
(PE-38408) Remove expensive Regexes from puppet profiler Java impl
2 parents 8eeb389 + f85021c commit acd44da

File tree

2 files changed

+106
-31
lines changed

2 files changed

+106
-31
lines changed

‎src/clj/puppetlabs/services/jruby/jruby_metrics_core.clj

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@
269269
(do
270270
(.update wait-timer
271271
(- (System/currentTimeMillis) (:time ta))
272-
(TimeUnit/MILLISECONDS))
272+
TimeUnit/MILLISECONDS)
273273
(swap! requested-instances dissoc requested-event))
274274
(log/warn (trs "Unable to find request event for borrowed JRuby instance: {0}" event))))
275275

@@ -281,8 +281,8 @@
281281
(if-let [ta (get @borrowed-instances worker-id)]
282282
(let [elapsed-time (- (System/currentTimeMillis) (:time ta))
283283
per-reason-timer (timer-for-borrow-reason metrics ta)]
284-
(.update borrow-timer elapsed-time (TimeUnit/MILLISECONDS))
285-
(.update per-reason-timer elapsed-time (TimeUnit/MILLISECONDS))
284+
(.update borrow-timer elapsed-time TimeUnit/MILLISECONDS)
285+
(.update per-reason-timer elapsed-time TimeUnit/MILLISECONDS)
286286
(MDC/put "jruby.borrow-time" (Long/toString elapsed-time))
287287
(swap! borrowed-instances dissoc worker-id))
288288
(log/warn (trs "JRuby instance ''{0}'' returned, but no record of when it was borrowed!" worker-id)))))
@@ -314,7 +314,7 @@
314314
(do
315315
(.update lock-wait-timer
316316
(- (System/currentTimeMillis) (:time lock-request))
317-
(TimeUnit/MILLISECONDS))
317+
TimeUnit/MILLISECONDS)
318318
(swap! lock-requests assoc
319319
lock-request-id
320320
{:state :acquired
@@ -330,7 +330,7 @@
330330
(do
331331
(.update lock-held-timer
332332
(- (System/currentTimeMillis) (:time lock-request))
333-
(TimeUnit/MILLISECONDS))
333+
TimeUnit/MILLISECONDS)
334334
(swap! lock-requests dissoc lock-request-id))
335335
(log/warn (trs "Lock request ''{0}'' released, but no record of when it was acquired!"
336336
lock-request-id)))

‎src/java/com/puppetlabs/puppetserver/MetricsPuppetProfiler.java

Lines changed: 101 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import org.apache.commons.lang.StringUtils;
66

77
import java.util.ArrayList;
8+
import java.util.Arrays;
89
import java.util.HashMap;
910
import java.util.List;
1011
import java.util.Map;
@@ -21,17 +22,21 @@ public class MetricsPuppetProfiler implements PuppetProfiler {
2122
private final MetricRegistry registry;
2223
private final Set<String> metric_ids;
2324

24-
private static final Pattern FUNCTION_PATTERN = Pattern.compile(".*\\.functions\\.([\\w\\d_]+)$");
25-
private static final Pattern RESOURCE_PATTERN = Pattern.compile(".*\\.compiler\\.evaluate_resource\\.([\\w\\d_]+\\[([\\w\\d_]+::)*[\\w\\d_]+\\])$");
26-
private static final Pattern CATALOG_PATTERN = Pattern.compile(".*\\.compiler\\.(static_compile_postprocessing|static_compile|compile|find_node)$");
27-
private static final Pattern INLINING_PATTERN = Pattern.compile(".*\\.compiler\\.static_compile_inlining\\.(.*)$");
28-
private static final Pattern PUPPETDB_PATTERN = Pattern.compile(".*\\.puppetdb\\.(resource\\.search|facts\\.encode|command\\.submit\\.replace facts|catalog\\.munge|command\\.submit\\.replace catalog|report\\.convert_to_wire_format_hash|command\\.submit\\.store report|query)$");
29-
25+
private final Map<String, Timer> function_timers;
26+
private final Map<String, Timer> resource_timers;
27+
private final Map<String, Timer> catalog_timers;
28+
private final Map<String, Timer> inlining_timers;
29+
private final Map<String, Timer> puppetdb_timers;
3030

3131
public MetricsPuppetProfiler(String hostname, MetricRegistry registry) {
3232
this.hostname = hostname;
3333
this.registry = registry;
3434
this.metric_ids = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
35+
this.function_timers = new ConcurrentHashMap<String, Timer>();
36+
this.resource_timers = new ConcurrentHashMap<String, Timer>();
37+
this.catalog_timers = new ConcurrentHashMap<String, Timer>();
38+
this.inlining_timers = new ConcurrentHashMap<String, Timer>();
39+
this.puppetdb_timers = new ConcurrentHashMap<String, Timer>();
3540
}
3641

3742
@Override
@@ -43,9 +48,12 @@ public Object start(String message, String[] metric_id) {
4348
public void finish(Object context, String message, String[] metric_id) {
4449
if (shouldTime(metric_id)) {
4550
Long elapsed = System.currentTimeMillis() - (Long)context;
46-
for (Timer t : getTimers(metric_id)) {
51+
Map<String, Timer> metricsByID = getOrCreateTimersByIDs(metric_id);
52+
for (Timer t : metricsByID.values()) {
4753
t.update(elapsed, TimeUnit.MILLISECONDS);
4854
}
55+
56+
updateMetricsTrackers(metric_id, metricsByID);
4957
}
5058
}
5159

@@ -54,29 +62,107 @@ public Set<String> getAllMetricIds() {
5462
}
5563

5664
public Map<String, Timer> getFunctionTimers() {
57-
return getTimers(FUNCTION_PATTERN);
65+
return this.function_timers;
5866
}
5967

6068
public Map<String, Timer> getResourceTimers() {
61-
return getTimers(RESOURCE_PATTERN);
69+
return this.resource_timers;
6270
}
6371

6472
public Map<String, Timer> getCatalogTimers() {
65-
return getTimers(CATALOG_PATTERN);
73+
return this.catalog_timers;
6674
}
6775

6876
public Map<String, Timer> getInliningTimers() {
69-
return getTimers(INLINING_PATTERN);
77+
return this.inlining_timers;
7078
}
7179

7280
public Map<String, Timer> getPuppetDBTimers() {
73-
return getTimers(PUPPETDB_PATTERN);
81+
return this.puppetdb_timers;
7482
}
7583

7684
@Override
7785
public void shutdown() {
7886
}
7987

88+
private List<String> sliceOfArrayToList(String[] idSegments, int lengthOfID) {
89+
// Callers expect a mutable List returned, but Arrays.asList() returns a
90+
// fix length array, which is why we have to create a List and then add to it.
91+
List<String> idList = new ArrayList<String>();
92+
idList.addAll(Arrays.asList(Arrays.copyOf(idSegments, lengthOfID)));
93+
94+
return idList;
95+
}
96+
97+
private String safeGet(String[] collection, int i) {
98+
try {
99+
return collection[i];
100+
} catch (IndexOutOfBoundsException _ex) {
101+
return "";
102+
}
103+
}
104+
105+
private void updateMetricsTrackers(String[] metricId, Map<String, Timer> metricsByID) {
106+
String firstElement = safeGet(metricId, 0);
107+
String secondElement = safeGet(metricId, 1);
108+
109+
if ("functions".equals(firstElement)) {
110+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
111+
this.function_timers.put(secondElement, metric);
112+
113+
} else if ("compiler".equals(firstElement)) {
114+
String thirdElemet = safeGet(metricId, 2);
115+
116+
if ("evaluate_resource".equals(secondElement)) {
117+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 3)));
118+
this.resource_timers.put(thirdElemet, metric);
119+
120+
} else if ("static_compile_inlining".equals(secondElement)) {
121+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 3)));
122+
this.inlining_timers.put(thirdElemet, metric);
123+
124+
} else {
125+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
126+
this.catalog_timers.put(secondElement, metric);
127+
}
128+
129+
} else if ("puppetdb".equals(firstElement)) {
130+
if ("query".equals(secondElement)) {
131+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 2)));
132+
this.puppetdb_timers.put(secondElement, metric);
133+
134+
} else {
135+
String thirdElemet = safeGet(metricId, 2);
136+
137+
if (
138+
("resource".equals(secondElement) && "search".equals(thirdElemet)) ||
139+
("payload".equals(secondElement) && "format".equals(thirdElemet)) ||
140+
// Set.of would be preferrable but 7.x still support Java 8, which does not have Set.of
141+
("facts".equals(secondElement) && Arrays.asList("save", "find", "search", "encode").contains(thirdElemet)) ||
142+
("catalog".equals(secondElement) && Arrays.asList("save", "munge").contains(thirdElemet)) ||
143+
("report".equals(secondElement) && Arrays.asList("convert_to_wire_format_hash", "process").contains(thirdElemet))
144+
) {
145+
String key = String.join(".", secondElement, thirdElemet);
146+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 3)));
147+
this.puppetdb_timers.put(key, metric);
148+
149+
} else if ("command".equals(secondElement) && "submit".equals(thirdElemet)) {
150+
String fourthElement = safeGet(metricId, 3);
151+
152+
if (
153+
"store report".equals(fourthElement) ||
154+
"replace facts".equals(fourthElement) ||
155+
"replace catalog".equals(fourthElement)
156+
) {
157+
String key = String.join(".", secondElement, thirdElemet, fourthElement);
158+
Timer metric = metricsByID.get(getMetricName(sliceOfArrayToList(metricId, 4)));
159+
this.puppetdb_timers.put(key, metric);
160+
}
161+
}
162+
}
163+
}
164+
}
165+
80166
private boolean shouldTime(String[] metric_id) {
81167
if (metric_id == null) {
82168
return false;
@@ -90,8 +176,8 @@ private boolean shouldTime(String[] metric_id) {
90176
return true;
91177
}
92178

93-
private List<Timer> getTimers(String[] metric_id) {
94-
List<Timer> timers = new ArrayList<Timer>();
179+
private Map<String, Timer> getOrCreateTimersByIDs(String[] metric_id) {
180+
Map<String, Timer> timers = new HashMap<String, Timer>();
95181
// If this is turns out to be a performance hit, we could cache these in a
96182
// map or something.
97183
for (int i = 0; i < metric_id.length; i++) {
@@ -101,7 +187,7 @@ private List<Timer> getTimers(String[] metric_id) {
101187
}
102188
String metric_name = getMetricName(current_id);
103189
registerMetricName(metric_name);
104-
timers.add(registry.timer(metric_name));
190+
timers.put(metric_name, registry.timer(metric_name));
105191
}
106192
return timers;
107193
}
@@ -114,15 +200,4 @@ private String getMetricName(List<String> metric_id) {
114200
private void registerMetricName(String metric_name) {
115201
this.metric_ids.add(metric_name);
116202
}
117-
118-
private Map<String, Timer> getTimers(Pattern pattern) {
119-
Map<String, Timer> rv = new HashMap<>();
120-
for (String metric_id : this.metric_ids) {
121-
Matcher matcher = pattern.matcher(metric_id);
122-
if (matcher.matches()) {
123-
rv.put(matcher.group(1), registry.timer(metric_id));
124-
}
125-
}
126-
return rv;
127-
}
128203
}

0 commit comments

Comments
 (0)