JAV-416 expose and publish metrics using servo #272
Conversation
| protected void scheduleInvocation() { | ||
| OperationMeta operationMeta = restOperationMeta.getOperationMeta(); | ||
| try { | ||
| Object[] args = RestCodec.restToArgs(requestEx, restOperationMeta); |
There was a problem hiding this comment.
this moved decode from executor to netThread, not so good.
prepareStartTimeMetrics only care about time and qualifiedName, qualifiedName can be got from operationMeta
prepareStartTimeMetrics can return a metrics object, and then save it to invocation in runOnExecutor.
|
|
||
| restProducerInvocation.runOnExecutor(); | ||
|
|
||
| restProducerInvocation.scheduleInvocation(); |
There was a problem hiding this comment.
infact, runOnExecutor triggered by scheduleInvocation
why invoke scheduleInvocation after runOnExecutor
|
|
||
| private AtomicLong queueStartTime = new AtomicLong(); | ||
|
|
||
| private AtomicLong endOpertime = new AtomicLong(); |
|
|
||
| private AtomicLong endOpertime = new AtomicLong(); | ||
|
|
||
| private AtomicLong queueEndtime = new AtomicLong(); |
| * Returns the time when the operation ends. | ||
| * @return long | ||
| */ | ||
| public long getEndOpertime() { |
| * Returns the time when it leaves the queue. | ||
| * @return long | ||
| */ | ||
| public long getQueueEndtime() { |
| * Sets the time when it leaves the queue. | ||
| * @param endtime Leaving time from queue | ||
| */ | ||
| public void setQueueEndtime(long endtime) { |
| /* list of monitors */ | ||
| List<Monitor<?>> monitors = getMetricsMonitors(); | ||
|
|
||
| MonitorConfig commandMetricsConfig = MonitorConfig.builder("CSE_" + "test name").build(); |
There was a problem hiding this comment.
Please keep sending the changes in this PR.
| this.invocation = InvocationFactory.forProvider(transport.getEndpoint(), | ||
| restOperationMeta.getOperationMeta(), | ||
| args); | ||
| MetricsDataMonitorUtil.prepareStartTimeMetrics(invocation); |
There was a problem hiding this comment.
why static call everywhere? it makes metrics implementation coupled with each module in which it's used.
not only unit testing has to involve metrics implementation, but also it makes switching underlying metrics implementation nearly impossible
| reqQueue.getTotalTime().addAndGet(timeInqueue); | ||
| reqQueue.incrementTotalCount(); | ||
|
|
||
| double avgTimeInQueue = reqQueue.getTotalTime().get() / reqQueue.getTotalCount().get(); |
There was a problem hiding this comment.
avg metrics can calculate once per cycle.
| double avgTimeInQueue = reqQueue.getTotalTime().get() / reqQueue.getTotalCount().get(); | ||
|
|
||
| // store average,min and max times in operation level first. | ||
| QueueMetrics requestMetricQueue = MetricsDataMonitor.getMetricsGlobalRef().getQueueMetrics(operPath); |
There was a problem hiding this comment.
concurrence problem?
and minTime/maxTime also have the problem?
| */ | ||
| public class MetricsServoRegistry implements InitializingBean { | ||
|
|
||
| private static final String METRICS_POLL_TIME = "cse.metrics.polltime"; |
There was a problem hiding this comment.
may declare time unit like "cse.metrics.polltime.second"
There was a problem hiding this comment.
its better to use only cse.metrics.polltime as using unit value in key is not good , as later we can support other time units also
| String fileName = DynamicPropertyFactory.getInstance().getStringProperty(FILENAME, "metrics").get(); | ||
| String filePath = DynamicPropertyFactory.getInstance().getStringProperty(FILEPATH, ".").get(); | ||
|
|
||
| MetricObserver fileObserver = new FileMetricObserver(fileName, new File(filePath)); |
There was a problem hiding this comment.
it's likely max,min and avg would never be reset ,I think they calculate latest 5min or 10min (can set) samples in a time window may better
There was a problem hiding this comment.
as of now support only one poll cycle , later we will support multiple cycles so that different rolling cycles can be supported as per user configuration
| } | ||
| requestMetricQueue.setAvgLifeTimeAfterQueue(avgWorkTimeAfterQueue); | ||
|
|
||
| double avgServiceExecTimeInstance = metricRef.getTotalServExecutionTime().get() |
There was a problem hiding this comment.
long must convert to double before division , please check all submit
| metricsRef.incrementTotalReqProvider(); | ||
| // note down metrics for operational level. | ||
| metricsRef.setOperMetTotalReq(operPath, | ||
| metricsRef.getOperMetTotalReq(operPath) == null ? 1L : metricsRef.getOperMetTotalReq(operPath) + 1); |
| tpsAndLatencyMap.put("TPS-" + operPath, String.valueOf(tpsOper)); | ||
| tpsAndLatencyMap.put("Latency-" + operPath, String.valueOf(operLatency)); | ||
| insTotalTps += tpsOper; | ||
| insTotalLatency += operLatency; |
There was a problem hiding this comment.
I think we can not direct plus all latency together，may plus (operLatency * rolling count percentage ) as result
|
i had rebase to lastest master , resolve conflict and fix unit test , new PR at #292 |
7 participants
No description provided.