HIVE-28505: OTEL: Implement OTEL Exporter to expose query details fro… #5451

tanishq-chugh · 2024-09-17T03:18:42Z

…m HiveServer2

What changes were proposed in this pull request?

Implement OTEL Exporter to expose basic query details from HiveServer2

Why are the changes needed?

Better Usability & introduce various OTEL Clients to fetch query data

Does this PR introduce any user-facing change?

OTEL Clients can be configured to fetch data from HS2

Is the change a dependency upgrade?

No

How was this patch tested?

Manual Testing by integrating it with OTEL collector & various receivers such as Jaeger, Zipkin.

…m HiveServer2

ayushtkn

Thanx @tanishq-chugh, have dropped some comments, please check & I think your code ain't properly formatted

ayushtkn · 2024-09-17T07:41:29Z

itests/hive-unit/pom.xml

+    <dependency>
+      <groupId>io.opentelemetry</groupId>
+      <artifactId>opentelemetry-exporter-otlp</artifactId>
+      <version>${otel.version}</version>
+    </dependency>


Why are we adding in itest? We are only having changes in the prod code

This is required in case of users running OTEL Service for HS2 which is running locally built from source code.

We don't package itests as part of the distribution, this is integration test module. HS2 should run locally without this itests, it ain't there in the class path either

ayushtkn · 2024-09-17T07:42:11Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+
+      while (lQuery.getQueryDisplay() == null) {
+        try {
+          Thread.sleep(500);


Why is there are sleep here?

In some cases, OTEL service was capturing the liveQueries in a state where queryDisplay was not initialised and leading to Null Pointer Exception. This is changed to drop such queries for the current OTEL run and capture it in next run.

ayushtkn · 2024-09-17T07:44:29Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+      if(queryIdToSpanMap.containsKey(queryID)){
+        Span rootspan = queryIdToSpanMap.get(queryID);


Span rootspan = queryIdToSpanMap.get(queryID); if (rootspan != null) {

ayushtkn · 2024-09-17T07:45:50Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+            queryIdToTasksMap.get(queryID).add(task.getTaskId());
+            Context parentContext = Context.current().with(rootspan);
+            Span currSpan = tracer.spanBuilder(queryID+ " - " + task.getTaskId() + " - live").setParent(parentContext).setAllAttributes(addTaskAttributes(task))
+                    .setStartTimestamp(task.getBeginTime(), TimeUnit.MILLISECONDS).startSpan();
+            while (task.getEndTime() == null){
+              try {
+                Thread.sleep(500);
+              } catch (InterruptedException e) {
+                throw new RuntimeException(e);
+              }


this is same as below, refactor into a method

Have refactored the code to remove the while loop of thread.sleep and usage of currSpan variable as well leading to single line of code for this block.

ayushtkn · 2024-09-17T07:46:52Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+
+            while (task.getEndTime() == null) {
+              try {
+                Thread.sleep(500);


We can't have sleep in the prod code like this, that too when the code is sequential, if it is null, just drop this task in this iteration and we will catch this in next iteration

ayushtkn · 2024-09-17T07:50:15Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        String hQueryId = hQuery.getQueryDisplay().getQueryId();
+        Span rootspan = queryIdToSpanMap.get(hQueryId);
+        for (QueryDisplay.TaskDisplay task : hQuery.getQueryDisplay().getTaskDisplays()) {
+          if(queryIdToTasksMap.get(hQueryId) == null || !queryIdToTasksMap.get(hQueryId).contains(task.getTaskId())){


can we use rootspan instead of queryIdToTasksMap.get(hQueryId)

queryIdToTasksMap.get(hQueryId) == null is not required anymore as this will never be null in case rootspan is not null. Have removed this condition.

ayushtkn · 2024-09-17T07:52:13Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+            Span currSpan = tracer.spanBuilder(hQueryId+ " - " + task.getTaskId() + " - completed").setParent(parentContext).setAllAttributes(addTaskAttributes(task))
+                    .setStartTimestamp(task.getBeginTime(), TimeUnit.MILLISECONDS).startSpan();
+            currSpan.end(task.getEndTime(), TimeUnit.MILLISECONDS);


I don't think we need to store currSpan we can chain the calls

tracer.spanBuilder(hQueryId + " - " + task.getTaskId() + " - completed") .setParent(parentContext).setAllAttributes(addTaskAttributes(task)) .setStartTimestamp(task.getBeginTime(), TimeUnit.MILLISECONDS).startSpan() .end(task.getEndTime(), TimeUnit.MILLISECONDS);

Made this change at all the other points as well.

ayushtkn · 2024-09-17T07:53:57Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        //Update the rootSpan name & attributes before ending it
+        queryIdToSpanMap.get(hQueryId).updateName(hQueryId + " - completed");
+        queryIdToSpanMap.get(hQueryId).setAllAttributes(addQueryAttributes(hQuery));
+        queryIdToSpanMap.get(hQueryId).end();


Does this work instead

//Update the rootSpan name & attributes before ending it rootspan.updateName(hQueryId + " - completed").setAllAttributes(addQueryAttributes(hQuery)).end();

ayushtkn · 2024-09-17T07:56:35Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+      if (!historicalQueryId.contains(hQuery.getQueryDisplay().getQueryId())) {
+        historicalQueryId.add(hQuery.getQueryDisplay().getQueryId());


It is a Set, you don't need to do contains then add, you can directly add and check the return value, if it is true, means the element wasn't there, if false, means the element was already there

ayushtkn · 2024-09-17T07:57:57Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        }
+        rootSpan.setAllAttributes(addQueryAttributes(hQuery)).end();
+      }
+


Your historicalQueryId will grow infinitely, you need to reset it the only ones returned by List<QueryInfo> historicalQueries = operationManager.getHistoricalQueryInfos();

This would return say only 25 (by default) but you would be storing it infinitely which will ultimately lead to OOM on the HS2 side

Yes, i have refactored the code such that at the end of each OTEL loop, all the queries which have been removed from historicalQueries will also be removed from our historicalQueryId.

ayushtkn · 2024-09-18T20:05:28Z

itests/hive-unit/pom.xml

+    <dependency>
+      <groupId>io.opentelemetry</groupId>
+      <artifactId>opentelemetry-exporter-otlp</artifactId>
+      <version>${otel.version}</version>
+    </dependency>


We don't package itests as part of the distribution, this is integration test module. HS2 should run locally without this itests, it ain't there in the class path either

ayushtkn · 2024-09-18T20:06:58Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+  private static final String INSTRUMENTATION_NAME = OTELExporter.class.getName();
+  private static final Logger LOG = LoggerFactory.getLogger(OTELExporter.class);
+  private final OperationManager operationManager;
+  private Set<String> historicalQueryId;


Should be final

ayushtkn · 2024-09-18T20:07:07Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+  private Map<String, Span> queryIdToSpanMap;
+  private Map<String, List<String>> queryIdToTasksMap;


should be final

ayushtkn · 2024-09-18T20:08:38Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+      //In case of live query previously encountered in past loops
+      if (rootspan != null) {
+        for (QueryDisplay.TaskDisplay task : lQuery.getQueryDisplay().getTaskDisplays()) {
+          if (task.getReturnValue() != null && task.getEndTime() != null && !queryIdToTasksMap.get(queryID).contains(task.getTaskId())) {


the line length is more than the allowed limit of 120

ayushtkn · 2024-09-18T20:09:24Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        rootspan = tracer.spanBuilder(queryID + " - live")
+                .startSpan();


nit

rootspan = tracer.spanBuilder(queryID + " - live").startSpan();

ayushtkn · 2024-09-18T20:18:11Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+                .startSpan();
+        Context parentContext = Context.current().with(rootspan);
+
+        Span initSpan = tracer.spanBuilder(hQuery.getQueryDisplay().getQueryId() + " - completed").setParent(parentContext).startSpan()


nit
line length is more than 120

ayushtkn · 2024-09-18T20:19:27Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+    attributes.put(AttributeKey.stringKey("Error Message"), query.getQueryDisplay().getErrorMessage());
+    attributes.put(AttributeKey.stringKey("explainPlan"), query.getQueryDisplay().getExplainPlan());
+    attributes.put(AttributeKey.stringKey("fullLogLocation"), query.getQueryDisplay().getFullLogLocation());
+    attributes.put(AttributeKey.stringKey("taskDisplays"), Joiner.on("\t").join(query.getQueryDisplay().getTaskDisplays()));


this ain't required IMO, we are having spans for each task now

Yes, removed taskDisplays attribute.

ayushtkn · 2024-09-18T20:20:19Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+
+  private AttributesMap addQueryAttributes(QueryInfo query){
+    AttributesMap attributes = AttributesMap.create(Long.MAX_VALUE, Integer.MAX_VALUE);
+    attributes.put(AttributeKey.stringKey("queryId"), query.getQueryDisplay().getQueryId());


I think queryId is already there right? if not q -> Q

ayushtkn · 2024-09-18T20:20:36Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+    attributes.put(AttributeKey.longKey("End Time"), query.getEndTime());
+    attributes.put(AttributeKey.stringKey("Operation Id"), query.getOperationId());
+    attributes.put(AttributeKey.stringKey("Operation Log Location"), query.getOperationLogLocation());
+    attributes.put(AttributeKey.stringKey("Error Message"), query.getQueryDisplay().getErrorMessage());


remove space

ayushtkn · 2024-09-18T20:21:54Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+                .setAttribute("queryID", hQuery.getQueryDisplay().getQueryId())
+                .setAttribute("queryString", hQuery.getQueryDisplay().getQueryString())
+                .setAttribute("Begin Time", hQuery.getBeginTime());


shouldn't we instead call addQueryAttributes here?

It would make initSpan and rootSpan storing the same content in case of historical queries. Would be better if we keep only these minimal attributes in initSpan so that there's consistency between live and historical traces.

ayushtkn · 2024-09-24T04:14:36Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        queryIdToSpanMap.put(queryID,rootspan);
+        queryIdToTasksMap.put(queryID,completedTasks);


this isn't formatted, can you format the code properly here & the other similar places

ayushtkn · 2024-09-24T04:15:44Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+      }
+    }
+
+    List<String> historicalQueryIDs = new ArrayList<>();


Should be a set

ayushtkn · 2024-09-24T04:33:33Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        Span initSpan = tracer.spanBuilder(queryID + " - live").setParent(parentContext).startSpan()
+                .setAttribute("QueryId", queryID)
+                .setAttribute("QueryString", lQuery.getQueryDisplay().getQueryString())
+                .setAttribute("BeginTime", lQuery.getBeginTime());


I think we should set setStartTimestamp for the intiSpan with this value rather than the attribute

ayushtkn · 2024-09-24T04:34:14Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        Context parentContext = Context.current().with(rootspan);
+
+        Span initSpan = tracer.spanBuilder(queryID + " - live").setParent(parentContext).startSpan()
+                .setAttribute("QueryId", queryID)


Add user as well

ayushtkn · 2024-09-24T04:37:14Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+
+        Span initSpan = tracer.spanBuilder(queryID + " - live").setParent(parentContext).startSpan()
+                .setAttribute("QueryId", queryID)
+                .setAttribute("QueryString", lQuery.getQueryDisplay().getQueryString())


can let queryString present only in rootSpan, some queries are huge to be put twice and are more eye catching as compared to just query id

Added queryString into initSpan and removed it from rootSpan

ayushtkn · 2024-09-24T04:40:04Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+        if (lQuery.getQueryDisplay().getErrorMessage() != null) {
+          initSpan.setAttribute("ErrorMessage", lQuery.getQueryDisplay().getErrorMessage());
+        }
+        initSpan.end();


this I believe should have the begin time as the end time here

ayushtkn

LGTM

ayushtkn · 2024-09-25T06:40:28Z

service/src/java/org/apache/hive/service/servlet/OTELExporter.java

+
+package org.apache.hive.service.servlet;
+
+import java.util.ArrayList;


unused import

my bad, removing it.

sonarcloud · 2024-09-25T07:59:31Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

HIVE-28505: OTEL: Implement OTEL Exporter to expose query details fro…

ecbcb87

…m HiveServer2

asf-ci-hive added tests pending tests passed and removed tests pending labels Sep 17, 2024

ayushtkn requested changes Sep 17, 2024

View reviewed changes

tanishq-chugh added 2 commits September 17, 2024 15:42

Add initSpan for live queries

f9400b0

Review comments

70bdcd6

asf-ci-hive added tests pending tests passed and removed tests passed tests pending labels Sep 17, 2024

ayushtkn reviewed Sep 18, 2024

View reviewed changes

Review comments - 2

89ba90a

asf-ci-hive added tests pending tests passed and removed tests passed tests pending labels Sep 19, 2024

ayushtkn reviewed Sep 24, 2024

View reviewed changes

Review comments - 3

d221a88

asf-ci-hive added tests pending tests failed tests passed and removed tests passed tests pending tests failed labels Sep 24, 2024

ayushtkn approved these changes Sep 25, 2024

View reviewed changes

Remove unused import

8ba1e9a

asf-ci-hive removed the tests passed label Sep 25, 2024

asf-ci-hive added the tests pending label Sep 25, 2024

asf-ci-hive added tests passed and removed tests pending labels Sep 25, 2024

ayushtkn merged commit 9d97e4a into apache:master Sep 27, 2024
5 checks passed

		if(queryIdToSpanMap.containsKey(queryID)){
		Span rootspan = queryIdToSpanMap.get(queryID);

		if (!historicalQueryId.contains(hQuery.getQueryDisplay().getQueryId())) {
		historicalQueryId.add(hQuery.getQueryDisplay().getQueryId());

		private Map<String, Span> queryIdToSpanMap;
		private Map<String, List<String>> queryIdToTasksMap;

		rootspan = tracer.spanBuilder(queryID + " - live")
		.startSpan();

		queryIdToSpanMap.put(queryID,rootspan);
		queryIdToTasksMap.put(queryID,completedTasks);


		package org.apache.hive.service.servlet;

		import java.util.ArrayList;

HIVE-28505: OTEL: Implement OTEL Exporter to expose query details fro… #5451

HIVE-28505: OTEL: Implement OTEL Exporter to expose query details fro… #5451

Conversation

tanishq-chugh commented Sep 17, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

ayushtkn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayushtkn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 25, 2024

Quality Gate passed