kyuubi icon indicating copy to clipboard operation
kyuubi copied to clipboard

[Improvement] Expose KyuubiOperation exec time metrics

Open beryllw opened this issue 2 years ago • 2 comments

Code of Conduct

Search before asking

  • [X] I have searched in the issues and found no similar issues.

What would you like to be improved?

Kyuubi already supports ExecuteStatement exec_time metrics. If KyuubiOperation (LaunchEngine, ExecutedCommandExec) supports exec_time metrics, it will be more convenient when troubleshooting. image

How should we improve?

KyuubiOperation (LaunchEngine, ExecutedCommandExec) supports exec_time metrics

Are you willing to submit PR?

  • [X] Yes. I would be willing to submit a PR with guidance from the Kyuubi community to improve.
  • [ ] No. I cannot submit a PR at this time.

beryllw avatar Jan 31 '24 03:01 beryllw

I also agree that these metrics would be very useful for troubleshooting.

I have noticed that in current implementation, metrics of ExecuteStatement exec_time is got within org.apache.kyuubi.operation.ExecuteStatement.

I wonder if we could move this kind of logic inside org.apache.kyuubi.operation.KyuubiOperation#setState to track all operation execution time.

  override def setState(newState: OperationState): Unit = {
    MetricsSystem.tracing { ms =>
      if (!OperationState.isTerminal(state)) {
        ms.markMeter(MetricRegistry.name(OPERATION_STATE, opType, state.toString.toLowerCase), -1)
        // add operation exec time tracking logic here
        val execTime = System.currentTimeMillis() - startTime
        ms.updateHistogram(
          MetricRegistry.name(OPERATION_EXEC_TIME, opType),
          execTime)
      }
      ms.markMeter(MetricRegistry.name(OPERATION_STATE, opType, newState.toString.toLowerCase))
      ms.markMeter(MetricRegistry.name(OPERATION_STATE, newState.toString.toLowerCase))
    }
    super.setState(newState)
    if (eventEnabled) EventBus.post(KyuubiOperationEvent(this))
  }

Z1Wu avatar Jan 31 '24 07:01 Z1Wu

extending the metrics scope to all operations makes sense to me, please go head to prepare the PR.

pan3793 avatar Jan 31 '24 07:01 pan3793