Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============7096014401278083993=="
MIME-Version: 1.0
Subject: Re: Review Request 54107: changes to intercept and time mybatis
 invocations
From: Joshua Cohen <jcohen@apache.org>
To: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>,
 David McLaughlin <david@dmclaughlin.com>,
 Mehrdad Nurolahzade <mehrdad@nurolahzade.com>,
 Joshua Cohen <jcohen@apache.org>
Cc: Aurora <reviews@aurora.apache.org>, Reza Motamedi <reza.motamedi@gmail.com>
Date: Tue, 29 Nov 2016 04:18:39 -0000
Message-ID: <20161129041839.5576.86889@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Joshua Cohen <noreply@reviews.apache.org>
References: <20161129023620.5576.68480@reviews.apache.org>
In-Reply-To: <20161129023620.5576.68480@reviews.apache.org>
Reply-To: Joshua Cohen <jcohen@apache.org>
archived-at: Tue, 29 Nov 2016 04:18:43 -0000

--===============7096014401278083993==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
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
> 
>


--===============7096014401278083993==--