Changing to use spectator for metrics #118

zhljen · 2017-05-19T15:20:26Z

No description provided.

ajoymajumdar · 2017-05-19T15:58:20Z

gradle.properties

@@ -37,4 +37,5 @@ hive_version=1.2.1

 ## speed up the build process
 #org.gradle.parallel=true
-org.gradle.jvmargs=-Xmx2G
+org.gradle.jvmargs=-Xmx4G


Cant we pass this as command arguments?

tgianos · 2017-05-19T15:50:35Z

...at-common-server/src/main/java/com/netflix/metacat/common/server/events/MetacatEventBus.java

@@ -60,7 +65,7 @@ public MetacatEventBus(
 */
 public void postAsync(final ApplicationEvent event) {
 log.debug("Received request to post an event {} asynchronously", event);
- CounterWrapper.incrementCounter("metacat.events.async");
+ registry.counter(LogConstants.CounterEventAsync.name()).increment();


Shouldn't it be called MetricConstants not LogConstants

tgianos · 2017-05-19T15:52:56Z

...at-common-server/src/main/java/com/netflix/metacat/common/server/events/MetacatEventBus.java

@@ -71,7 +76,7 @@ public void postAsync(final ApplicationEvent event) {
 */
 public void postSync(final ApplicationEvent event) {
 log.debug("Received request to post an event {} synchronously", event);
- CounterWrapper.incrementCounter("metacat.events.sync");
+ registry.counter(LogConstants.CounterEventSync.name()).increment();


Is it better to do this than to just have a counter reference saved in the constructor as a private member so we don't have to get the name every time? Then we just call increment on the local reference

The spectator itself has the counter reference cached. The counter only works in active mode and introducing a local id variable will not save. The gauge is the only one that has passive mode, where caches the reference at application will be useful.

tgianos · 2017-05-19T15:53:57Z

...t-common-server/src/main/java/com/netflix/metacat/common/server/monitoring/LogConstants.java

+ /**
+ * evnets.
+ */
+ CounterEventAsync(AppPrefix + ".events.count.Async"),


it seems to me that Async vs. Sync could be a tag rather than part of the name.

I think it's ok to let them be a separated counter for consistency. There're other cases like this, e.g. tableCreate, tableDelete, etc. A couple of cased the tags used are (1) the status of operations, failure/succeed; (2) the catalog/database/table names in case of the same operation.

tgianos · 2017-05-19T15:54:22Z

...t-common-server/src/main/java/com/netflix/metacat/common/server/monitoring/LogConstants.java

+
+import java.util.Map;
+
+//CHECKSTYLE:OFF
 /**
 * Log constants.
 */
 public enum LogConstants {


Rename to MetricsConstants and why is it an enumeration instead of just a class of string constants?

It can be defined as a constant class. But it also has methods, and needs to be a singleton. Enum seems ok. The naming is bit misleading, and should be renamed as Metrics

tgianos · 2017-05-19T15:55:24Z

...t-common-server/src/main/java/com/netflix/metacat/common/server/monitoring/LogConstants.java

+ /**
+ * Notifications.
+ */
+ CounterSNSNotificationPartitionAdd(AppPrefix + ".notifications.count.partitionsAdd"),


I might move count to the end. like partitionsAdd.count

This is for the consistency of naming, i.e. the component.spectatorType.measureName. The type can be count, gauge, timer.

I think we should flip it so it's component.measureName.type... I say this cause when we type it into the atlas search filter later on we can better filter down by the metrics and we'll then see that metric name will have a counter, gauge or timer associated with it as opposed to having to back out from the metric to see other metrics cause they're grouped under type... @ajoymajumdar opinion?

tgianos · 2017-05-19T15:56:03Z

...t-common-server/src/main/java/com/netflix/metacat/common/server/monitoring/LogConstants.java

+ CounterElasticSearchDelete(AppPrefix + ".elasticsearch.count.esDelete"),
+ CounterElasticSearchBulkDelete(AppPrefix + ".elasticsearch.count.esBulkDelete"),
+ CounterElasticSearchUpdate(AppPrefix + ".elasticsearch.count.esUpdate"),
+ CounterElasticSearchBulkUpdate(AppPrefix + ".elasticsearch.count.esBulkUpdate"),


isn't having it elasticsearch and then es prefix on the name itself kind of redundant?

This is to separated the operations from metacat e.g. database/table or elasticSearch itself. The elasticsearch prefix indicates the components, and the 'es' indicates the operation target.

tgianos · 2017-05-19T15:58:29Z

...common-server/src/main/java/com/netflix/metacat/common/server/util/PoolStatsInterceptor.java

- DynamicGauge.set(metricNameActive, parent.getActive());
- DynamicGauge.set(metricNameIdle, parent.getIdle());
+ if (parent != null
+ && metricNameTotalGauage != null


Rather than doing all these null checks why wouldn't you just instantiate the gauges once in the constructor and make them final member variables?

We cannot since Tomcat pool implementation needs a default empty constructor.

They can't be instantiated in the constructor, as the "metricName" are unknown at that time.

tgianos · 2017-05-19T15:59:27Z

...at-connector-hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFactory.java

+ private IMetacatHiveClient createLocalClient() throws Exception {
+ final HiveConf conf = getDefaultConf();
+ configuration.forEach(conf::set);
+ //TO DO Change the usage of DataSourceManager later


really minor but you should make this TODO one word so we can find it quickly and IDE's will pick it up

tgianos · 2017-05-19T16:00:09Z

...hive/src/main/java/com/netflix/metacat/connector/hive/HiveConnectorFastPartitionService.java

 log.info("###### Time taken to complete "
- + "HiveConnectorFastPartitionService.getPartitions is {} ms", timer.stop());
+ + "HiveConnectorFastPartitionService.getPartitions is {} ms", duration);
+ this.registry.timer(LogConstants.TimerHiveGetPartitions.name()).record(duration, TimeUnit.MILLISECONDS);


keep reference to timer

Similar as the counter explanation.

tgianos · 2017-05-19T16:02:07Z

metacat-thrift/src/main/java/com/netflix/metacat/thrift/CatalogThriftHiveMetastore.java

+ final long duration = registry.clock().monotonicTime() - start;
+ this.registry.timer(LogConstants.TimerThriftRequest.name()
+ + "." + methodName).record(duration, TimeUnit.MILLISECONDS);
+ log.info("+++ Thrift({}): Time taken to complete {} is {} ms", catalogName, methodName, duration);


Do we really want all this to be info level? Seems to me they might be better as debug? Not sure how @ajoymajumdar has this overall

Info is fine. All request methods log info messages at the start and end of the request.

Zhen Li added 2 commits May 19, 2017 07:27

Changing metric collection to spring spectator apis.

421fe1b

removing unneeded registry and renaming constants

a4910bc

zhljen requested review from tgianos and ajoymajumdar May 19, 2017 15:20

zhljen added this to the 1.1.0 milestone May 19, 2017

zhljen added the enhancement label May 19, 2017

tgianos assigned zhljen May 19, 2017

ajoymajumdar reviewed May 19, 2017

View reviewed changes

tgianos suggested changes May 19, 2017

View reviewed changes

Changes to address review comments

db9d312

ajoymajumdar approved these changes May 19, 2017

View reviewed changes

tgianos approved these changes May 19, 2017

View reviewed changes

tgianos merged commit 32a6ee4 into Netflix:master May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing to use spectator for metrics #118

Changing to use spectator for metrics #118

zhljen commented May 19, 2017

ajoymajumdar May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

ajoymajumdar May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

zhljen May 19, 2017

tgianos May 19, 2017

ajoymajumdar May 19, 2017

Changing to use spectator for metrics #118

Changing to use spectator for metrics #118

Conversation

zhljen commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment