----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54107/#review157197 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 74) <https://reviews.apache.org/r/54107/#comment227645> fix class name for logger. src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (lines 97 - 99) <https://reviews.apache.org/r/54107/#comment227647> Just inline this into the LoadingCache above? src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 107) <https://reviews.apache.org/r/54107/#comment227649> We generally try and avoid log statements in this type of voice, replace "I'll use metric name..." with something like, "Using metric name..." Also, we recently patched all of our log statements to use replacement tokens rather than string concat (to avoid costly string operations for logs that are not emitted due to logging levels). Please replace the string concat here with `LOG.warn("... Using metric name '{}' instead.", INVALID_INVOCATION_METRIC_NAME)` src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java (line 119) <https://reviews.apache.org/r/54107/#comment227655> What is the performance impact of this interceptor? Do we want to intercept every call? Should we instead allow for some configurable sampling percentage? src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (line 36) <https://reviews.apache.org/r/54107/#comment227652> Can you add some negative test cases as well (e.g. if the first parameter is not an `MappedStatement`)? src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (line 46) <https://reviews.apache.org/r/54107/#comment227651> You shouldn't need to create a mock of `FakeClock`, it's already itself essentially a mock. Instead, the generally usage is along the lines of... FakeClock clock = new FakeClock(); ... // advance the clock by 1 second clock.advance(Amount.of(1L, Time.SECONDS)); ... // Do some more work // Advance the clock some more... clock.advance(Amount.of(10L, Time.SECONDS)); ... // Reset the clock... clock.setNowMillis(0L); If you search through the codebase you should be able to find copious examples to crib from. src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java (lines 52 - 54) <https://reviews.apache.org/r/54107/#comment227650> `expect(invocation.getArgs()).andReturn(args).times(3)` src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java (line 52) <https://reviews.apache.org/r/54107/#comment227653> Use `//` style comments, no need for javadoc style here. src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java (line 42) <https://reviews.apache.org/r/54107/#comment227654> same here. - Joshua Cohen On Nov. 29, 2016, 2:36 a.m., Reza Motamedi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54107/ > ----------------------------------------------------------- > > (Updated Nov. 29, 2016, 2:36 a.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, Mehrdad > Nurolahzade, and Santhosh Kumar Shanmugham. > > > Repository: aurora > > > Description > ------- > > MyBatis allows us to intercept calls within the execution of a mapped > statement. This allows us to time various mapped statements and ultimately > gain more insight on the performance of the database module. > > This patch introduces an interceptor on MyBatis on `updates` and `query` > mapped statements. I used the following convention to create name for the > newly collected stats: > mybatis.<<the id of the mapped statement>> > > After interception the process is very similar to the one in > @Timed-interceptor. SlidingStats can be used to export interval averages, > total milliseconds and the event counts. > > __example stats (from ./vars.json)__ > mybatis.create_tables_events 1 > mybatis.create_tables_events_per_sec 0.0 > mybatis.create_tables_nanos_per_event 0.0 > mybatis.create_tables_nanos_total 592633784 > mybatis.create_tables_nanos_total_per_sec 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_events > 3 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_events_per_sec > 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_per_event > 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_total > 2858362 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.selectAll_nanos_total_per_sec > 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_events > 333 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_events_per_sec > 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_per_event > 0.0 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_total > 85745680 > mybatis.org.apache.aurora.scheduler.storage.db.AttributeMapper.select_nanos_total_per_sec > 0.0 > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java > e7287cec28e7b8ca978c506bfe821f261bc0ac26 > > src/main/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptor.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/db/InstrumentingInterceptorTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java > 2e560c0d565689703b282391fe49dbf213ee25dc > > src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java > 79999e19454568540c14f91808635bf8dff82bb3 > > Diff: https://reviews.apache.org/r/54107/diff/ > > > Testing > ------- > > Tests are covered in InstrumentingInterceptorTest. > > - testStatIsCreatedOnce > Tests that each stat is created one time only. > > - testInterceptMarksMetrics > Tests that invocation is correctly intercepted and then proceeds. > > > Thanks, > > Reza Motamedi > >
