ayushtkn commented on code in PR #5535:
URL: https://github.com/apache/hive/pull/5535#discussion_r2909404826
##########
ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java:
##########
@@ -305,6 +309,26 @@ public synchronized void setExplainPlan(String
explainPlan) {
this.explainPlan = explainPlan;
}
+ public synchronized void setTezCounters(TezCounters tc) {
+ this.tezCounters = tc;
+ }
+
+ public synchronized TezCounters getTezCounters() {
+ return this.tezCounters;
+ }
+
+ public synchronized String getCountersAsString() {
+ JSONObject countersJson = new JSONObject();
+ if (tezCounters != null){
+ for (CounterGroup group : tezCounters) {
+ for (TezCounter counter : group) {
+ countersJson.put(group.getName() + " - " + counter.getDisplayName(),
counter.getValue());
+ }
+ }
+ }
+ return countersJson.toString();
+ }
Review Comment:
Can we use `Jackson` `ObjectMapper` instead, To best of my knowledge:
Jackson's `ObjectNode` is generally more memory-efficient than
`org.json.JSONObject` when handling large sets of counters.
Jackson even handles nulls and special characters in counter names more
gracefully, ensuring the resulting string is always valid JSON.
The `ObjectMapper` can be cached as `private static` at the class level and
can be used for all the calls
##########
ql/src/java/org/apache/hadoop/hive/ql/QueryDisplay.java:
##########
@@ -305,6 +309,26 @@ public synchronized void setExplainPlan(String
explainPlan) {
this.explainPlan = explainPlan;
}
+ public synchronized void setTezCounters(TezCounters tc) {
+ this.tezCounters = tc;
+ }
+
+ public synchronized TezCounters getTezCounters() {
+ return this.tezCounters;
+ }
+
+ public synchronized String getCountersAsString() {
+ JSONObject countersJson = new JSONObject();
+ if (tezCounters != null){
+ for (CounterGroup group : tezCounters) {
+ for (TezCounter counter : group) {
+ countersJson.put(group.getName() + " - " + counter.getDisplayName(),
counter.getValue());
Review Comment:
As of now it is a flat JSON
```
{
org.apache.tez.common.counters.DAGCounter - NUM_SUCCEEDED_TASKS:2,
org.apache.tez.common.counters.DAGCounter - TOTAL_LAUNCHED_TASKS:2,
org.apache.tez.common.counters.DAGCounter -
DURATION_SUCCEEDED_TASKS_MILLIS:571,
org.apache.tez.common.counters.DAGCounter - RACK_LOCAL_TASKS:1,
org.apache.tez.common.counters.DAGCounter - AM_CPU_MILLISECONDS:2130,
org.apache.tez.common.counters.DAGCounter - WALL_CLOCK_MILLIS:571,
org.apache.tez.common.counters.DAGCounter - AM_GC_TIME_MILLIS:10,
org.apache.tez.common.counters.DAGCounter - INITIAL_HELD_CONTAINERS:0,
org.apache.tez.common.counters.DAGCounter - TOTAL_CONTAINERS_USED:2,
org.apache.tez.common.counters.DAGCounter - TOTAL_CONTAINER_LAUNCH_COUNT:2,
org.apache.tez.common.counters.DAGCounter - TOTAL_CONTAINER_RELEASE_COUNT:2,
org.apache.tez.common.counters.DAGCounter - NODE_USED_COUNT:1,
org.apache.tez.common.counters.DAGCounter - NODE_TOTAL_COUNT:1,
org.apache.tez.common.counters.TaskCounter - SPILLED_RECORDS:0,
org.apache.tez.common.counters.TaskCounter - NUM_SHUFFLED_INPUTS:0,
org.apache.tez.common.counters.TaskCounter - NUM_FAILED_SHUFFLE_INPUTS:0,
org.apache.tez.common.counters.TaskCounter - INPUT_RECORDS_PROCESSED:5,
org.apache.tez.common.counters.TaskCounter - INPUT_SPLIT_LENGTH_BYTES:1,
org.apache.tez.common.counters.TaskCounter - OUTPUT_RECORDS:1,
org.apache.tez.common.counters.TaskCounter - APPROXIMATE_INPUT_RECORDS:1,
org.apache.tez.common.counters.TaskCounter - OUTPUT_LARGE_RECORDS:0,
org.apache.tez.common.counters.TaskCounter - OUTPUT_BYTES:37,
org.apache.tez.common.counters.TaskCounter - OUTPUT_BYTES_WITH_OVERHEAD:45,
org.apache.tez.common.counters.TaskCounter - OUTPUT_BYTES_PHYSICAL:73,
org.apache.tez.common.counters.TaskCounter -
ADDITIONAL_SPILLS_BYTES_WRITTEN:0,
org.apache.tez.common.counters.TaskCounter - ADDITIONAL_SPILLS_BYTES_READ:0,
org.apache.tez.common.counters.TaskCounter - ADDITIONAL_SPILL_COUNT:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_BYTES:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_BYTES_DECOMPRESSED:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_BYTES_TO_MEM:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_BYTES_TO_DISK:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_BYTES_DISK_DIRECT:0,
org.apache.tez.common.counters.TaskCounter - SHUFFLE_PHASE_TIME:14,
org.apache.tez.common.counters.TaskCounter - FIRST_EVENT_RECEIVED:14,
org.apache.tez.common.counters.TaskCounter - LAST_EVENT_RECEIVED:14,
org.apache.tez.common.counters.TaskCounter - DATA_BYTES_VIA_EVENT:49,
HIVE - CREATED_FILES:2,
HIVE - DESERIALIZE_ERRORS:0,
HIVE - RECORDS_IN_Map_1:3,
HIVE - RECORDS_OUT_0:1,
HIVE - RECORDS_OUT_1_default.emp:4,
HIVE - RECORDS_OUT_INTERMEDIATE_Map_1:1,
HIVE - RECORDS_OUT_INTERMEDIATE_Reducer_2:0,
HIVE - RECORDS_OUT_OPERATOR_FS_15:1,
HIVE - RECORDS_OUT_OPERATOR_FS_4:4,
HIVE - RECORDS_OUT_OPERATOR_GBY_13:1,
HIVE - RECORDS_OUT_OPERATOR_GBY_7:1,
HIVE - RECORDS_OUT_OPERATOR_MAP_0:0,
HIVE - RECORDS_OUT_OPERATOR_RS_8:1,
HIVE - RECORDS_OUT_OPERATOR_SEL_1:1,
HIVE - RECORDS_OUT_OPERATOR_SEL_14:1,
HIVE - RECORDS_OUT_OPERATOR_SEL_3:4,
HIVE - RECORDS_OUT_OPERATOR_SEL_6:4,
HIVE - RECORDS_OUT_OPERATOR_TS_0:1,
HIVE - RECORDS_OUT_OPERATOR_UDTF_2:4,
HIVE - TOTAL_TABLE_ROWS_WRITTEN:4
```
can we rather group them at group level?
##########
service/src/java/org/apache/hive/service/servlet/OTELExporter.java:
##########
@@ -214,6 +214,7 @@ private AttributesMap addQueryAttributes(QueryInfo query){
attributes.put(AttributeKey.stringKey("UserName"), query.getUserName());
attributes.put(AttributeKey.stringKey("State"), query.getState());
attributes.put(AttributeKey.stringKey("SessionId"), query.getSessionId());
+ attributes.put(AttributeKey.stringKey("TezCounters"),
query.getQueryDisplay().getCountersAsString());
Review Comment:
if we don't have the counters, I don't believe we need to expose empty
attributes, we can skip it based on the conf or null check or so
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -5759,7 +5759,10 @@ public static enum ConfVars {
HIVE_OTEL_RETRY_BACKOFF_MULTIPLIER("hive.otel.retry.backoff.multiplier",
5f,
"The multiplier applied to the backoff interval for retries in the
OTEL exporter."
- + "This determines how much the backoff interval increases after
each failed attempt, following an exponential backoff strategy.");
+ + "This determines how much the backoff interval increases after
each failed attempt, following an exponential backoff strategy."),
+
+ HIVE_OTEL_EXPOSE_TEZ_COUNTERS("hive.otel.expose.tez.counters", false,
+ "Enables the inclusion of Tez counters in OTEL output for a particular
query. Default is false.");
Review Comment:
can you change the name to `hive.otel.export.tez.counters` and no need to
mention the default is false in text
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
