-
Notifications
You must be signed in to change notification settings - Fork 40
[ES-1436915] Add additional null check for connection-context #810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
688cf75
2e6bfaa
8ee0fa1
5f7d0c1
2e28c9e
a720802
e88657b
2670e43
c4925ee
5a5f1de
bf41453
b81f4ad
1e790a4
5ed0c08
73029b4
ffc2aae
7cf9cc8
f55ca04
ceb2fcb
77552fc
0690dea
638a11a
a72870e
99c83f8
5da3b1e
39ee46b
3d03893
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,44 +113,54 @@ static void exportLatencyLog( | |
| long latencyMilliseconds, | ||
| SqlExecutionEvent executionEvent, | ||
| StatementId statementId) { | ||
| TelemetryEvent telemetryEvent = | ||
| new TelemetryEvent() | ||
| .setLatency(latencyMilliseconds) | ||
| .setSqlOperation(executionEvent) | ||
| .setDriverConnectionParameters(getDriverConnectionParameter(connectionContext)); | ||
| if (statementId != null) { | ||
| telemetryEvent.setSqlStatementId(statementId.toString()); | ||
| // Though we already handle null connectionContext in the downstream implementation, | ||
| // we are adding this check for extra sanity | ||
| if (connectionContext != null) { | ||
| TelemetryEvent telemetryEvent = | ||
| new TelemetryEvent() | ||
| .setLatency(latencyMilliseconds) | ||
| .setSqlOperation(executionEvent) | ||
| .setDriverConnectionParameters(getDriverConnectionParameter(connectionContext)); | ||
| if (statementId != null) { | ||
| telemetryEvent.setSqlStatementId(statementId.toString()); | ||
| } | ||
| TelemetryFrontendLog telemetryFrontendLog = | ||
| new TelemetryFrontendLog() | ||
| .setFrontendLogEventId(getEventUUID()) | ||
| .setContext(getLogContext()) | ||
| .setEntry(new FrontendLogEntry().setSqlDriverLog(telemetryEvent)); | ||
| TelemetryClientFactory.getInstance() | ||
| .getTelemetryClient( | ||
| connectionContext, DatabricksThreadContextHolder.getDatabricksConfig()) | ||
| .exportEvent(telemetryFrontendLog); | ||
| } | ||
| TelemetryFrontendLog telemetryFrontendLog = | ||
| new TelemetryFrontendLog() | ||
| .setFrontendLogEventId(getEventUUID()) | ||
| .setContext(getLogContext()) | ||
| .setEntry(new FrontendLogEntry().setSqlDriverLog(telemetryEvent)); | ||
| TelemetryClientFactory.getInstance() | ||
| .getTelemetryClient(connectionContext, DatabricksThreadContextHolder.getDatabricksConfig()) | ||
| .exportEvent(telemetryFrontendLog); | ||
| } | ||
|
|
||
| public static void exportLatencyLog( | ||
| IDatabricksConnectionContext connectionContext, | ||
| long latencyMilliseconds, | ||
| DriverVolumeOperation volumeOperationEvent) { | ||
| TelemetryFrontendLog telemetryFrontendLog = | ||
| new TelemetryFrontendLog() | ||
| .setFrontendLogEventId(getEventUUID()) | ||
| .setContext(getLogContext()) | ||
| .setEntry( | ||
| new FrontendLogEntry() | ||
| .setSqlDriverLog( | ||
| new TelemetryEvent() | ||
| .setLatency(latencyMilliseconds) | ||
| .setVolumeOperation(volumeOperationEvent) | ||
| .setDriverConnectionParameters( | ||
| getDriverConnectionParameter(connectionContext)))); | ||
| // Though we already handle null connectionContext in the downstream implementation, | ||
| // we are adding this check for extra sanity | ||
| if (connectionContext != null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should introduce this weird ad-hoc check somewhere deep in the code when we are actually exporting the logs. This is happening already. So the PR is a no-op
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we are already doing that. It is just that to make this future proof, where we accidently add dependency on connectionContext in any downstream logic and breaking the functionality. Telemetry is common to all executions. In current impl you're right that this is a no-op. |
||
| TelemetryFrontendLog telemetryFrontendLog = | ||
| new TelemetryFrontendLog() | ||
| .setFrontendLogEventId(getEventUUID()) | ||
| .setContext(getLogContext()) | ||
| .setEntry( | ||
| new FrontendLogEntry() | ||
| .setSqlDriverLog( | ||
| new TelemetryEvent() | ||
| .setLatency(latencyMilliseconds) | ||
| .setVolumeOperation(volumeOperationEvent) | ||
| .setDriverConnectionParameters( | ||
| getDriverConnectionParameter(connectionContext)))); | ||
|
|
||
| TelemetryClientFactory.getInstance() | ||
| .getTelemetryClient(connectionContext, DatabricksThreadContextHolder.getDatabricksConfig()) | ||
| .exportEvent(telemetryFrontendLog); | ||
| TelemetryClientFactory.getInstance() | ||
| .getTelemetryClient( | ||
| connectionContext, DatabricksThreadContextHolder.getDatabricksConfig()) | ||
| .exportEvent(telemetryFrontendLog); | ||
| } | ||
| } | ||
|
|
||
| private static DriverConnectionParameters getDriverConnectionParameter( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check DatabricksConfig too?
DatabricksThreadContextHolder.getDatabricksConfigThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed for authenticated client. Added the check there.