Add more metric reporting to MSQ

[adarshsanjeev]

Adds reporting of additional metrics and dimensions to MSQ task and Dart.

Summary
Currently, MSQ reports the a few metrics. (ingest/tombstones/count, ingest/segments/count and ingest/input/bytes).

This PR adds a few new metrics:

• query/time: Reported by controller and worker at the end of the query.
• query/cpu/time: Reported by each worker at the end of the query.

Additionally, adds some more dimensions to the metrics emitted. Dimensions added to metrics:

• queryId
• sqlQueryId
• engine: Denotes the engine used for the query, msq-dart or msq-task.
• dartQueryId: (Dart only)
• type: Always msq
• dataSource
• interval
• duration
• success

---

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[gianm]

Just a few line comments. Please also include tests that verify the controller and workers are emitting the correct metrics with the correct dimensions. One way you can do that is:

• update MSQTestControllerContext and MSQTestWorkerContext to have emitMetric write emitted metrics into some list
• provide a mechanism for test cases to get the emitted metrics

[HiveQA]

I still feel that the context#emitMetric method is odd; it could be something like:

    MsqServiceMetricEventBuilder metricBuilder = new MsqServiceMetricEventBuilder(querySpec);

    metricBuilder.setDimension(DruidMetrics.DATASOURCE, DefaultQueryMetrics.getTableNamesAsString(datasources));
    metricBuilder.setDimension(DruidMetrics.INTERVAL, DefaultQueryMetrics.getIntervalsAsStringArray(intervals));
    metricBuilder.setDimension(DruidMetrics.DURATION, BaseQuery.calculateDuration(intervals));
    metricBuilder.setDimension(DruidMetrics.SUCCESS, success);

    metricBuilder.setMetric("query/time", startTime - System.currentTimeMillis());
    context.getEmitter().emit(metricBuilder);

probably MsqServiceMetricEventBuilder could also provide a more natural home for those static methods which could become instance methods

..or alter the signature of context#emitrMetric to context#emit(metricBuilder) ? so that the context could still
add stuff if it wants - but the caller has the control.

[kgyrtkirk]

sorry about the above author :D it was me...I was just checking some github features ; which involved switching between 2 users ....

[gianm]

Ah, yes, that would work too. It's kind of equivalent (the metricBuilder has a metric name, value, and dimensions in it) but bundled together more nicely. @adarshsanjeev - could you try this approach? It looks like it could be cleaner.

[adarshsanjeev]

@kgyrtkirk Made these changes. The function now takes a service emitter.

There's an awkward bit in TaskControllerContext, where we have to check if the dataSource has already been provided in the metricBuilder before setting the default value, for backward compatibility. I wonder if it is worth adding yet another function of getDefaultMetricBuilder() from the context instead, so that the caller can decide what all to override instead of this weird code.

Please let me know your thoughts.

[kgyrtkirk]

thank you for the updates! looks good
left a few notes here and there - the IT failures are unrelated

[kgyrtkirk]

nit: is this a one time need - or maybe there should be queryDef.getDatasources ?

[adarshsanjeev]

Currently, I believe this is a one time need. Now that I have changed getDatasources to a member of stageDefinition, maybe this isn't required.