Skip to content

Commit 1236e7f

Browse files
authored
Fix null statementID in telemetry logs + make thread daemon (#938)
## Description - Currently telemetry wont be sent for cases where operationHandle is not present, this PR fixes that. Example instance where our telemetry would be useless : https://databricks.atlassian.net/browse/PECOBLR-359 - Also made telemetry client async : this avoids JVM to be alive even after main program completes ## Testing <!-- Describe how the changes have been tested--> ## Additional Notes to the Reviewer The daemon introduces a known bug that will be fixed in a later PR https://databricks.atlassian.net/browse/PECO-2716 - We have set the telemetry thread as daemon thread in JDBC. But, if the connection is closed and program is ended, the thread may not be able to send telemetry logs. There are 2 solutions discussed within the team on this (but we would need benchmarking/ analyse the pros cons on this to conclude) : - Make close telemetry client flush sync in nature - shutdown hook added to the executor - A new HTTP Client may be created on the conn.close because HTTP Client corresponding to the connectionContext is removed but the telemetry task may not be complete by then(This will be resolved by fixing the above issue 1.a. But we will have to finalize this too) NO_CHANGELOG=true
1 parent 8c44a07 commit 1236e7f

File tree

3 files changed

+10
-10
lines changed

3 files changed

+10
-10
lines changed

src/main/java/com/databricks/jdbc/telemetry/TelemetryClientFactory.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ private static ThreadFactory createThreadFactory() {
3737
@Override
3838
public Thread newThread(Runnable r) {
3939
Thread thread = new Thread(r, "Telemetry-Thread-" + threadNumber.getAndIncrement());
40-
thread.setDaemon(false);
40+
// TODO : https://databricks.atlassian.net/browse/PECO-2716
41+
thread.setDaemon(true);
4142
return thread;
4243
}
4344
};

src/main/java/com/databricks/jdbc/telemetry/TelemetryHelper.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,14 @@ public static void exportFailureLog(
125125
String errorMessage,
126126
String statementId,
127127
Long chunkIndex) {
128-
StatementTelemetryDetails telemetryDetails =
129-
TelemetryCollector.getInstance().getOrCreateTelemetryDetails(statementId);
130128
DriverErrorInfo errorInfo =
131129
new DriverErrorInfo().setErrorName(errorName).setStackTrace(errorMessage);
130+
StatementTelemetryDetails telemetryDetails;
131+
if (statementId == null) {
132+
telemetryDetails = new StatementTelemetryDetails(null);
133+
} else {
134+
telemetryDetails = TelemetryCollector.getInstance().getOrCreateTelemetryDetails(statementId);
135+
}
132136
exportTelemetryEvent(connectionContext, telemetryDetails, errorInfo, chunkIndex);
133137
}
134138

src/test/java/com/databricks/jdbc/telemetry/TelemetryHelperTest.java

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -161,18 +161,13 @@ void testExportTelemetryLogWithNullDetails() {
161161
}
162162

163163
@Test
164-
void testExportFailureLogWithNullContext() {
164+
void testExportFailureLogWithNullContextAndNullStatementId() {
165165
// Clear thread context to test with null context
166166
DatabricksThreadContextHolder.clearConnectionContext();
167+
DatabricksThreadContextHolder.clearStatementInfo();
167168
assertDoesNotThrow(() -> TelemetryHelper.exportFailureLog(null, "err", "msg"));
168169
}
169170

170-
@Test
171-
void testExportFailureLogWithNullStatementId() {
172-
// Skip this test as it causes infinite recursion
173-
// The test would verify that exportFailureLog handles null statement ID
174-
}
175-
176171
@Test
177172
public void testGetDatabricksConfigSafely_ReturnsNullOnError() {
178173
// Clear thread context to avoid telemetry export during test

0 commit comments

Comments
 (0)