databricks adbc driver telemetry lld #3624
Conversation
Signed-off-by: Sreekanth Vadigi <[email protected]>
| ## 3. Architecture Overview | ||
|
|
||
| ### 3.1 High-Level Design | ||
|
|
There was a problem hiding this comment.
Please make this a mermaid diagram
| │ - /telemetry-unauth (unauthenticated - connection errors) │ | ||
| └─────────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| ▼ |
There was a problem hiding this comment.
please remove the databricsk internal implementation
| /// Collects and aggregates telemetry events for a connection. | ||
| /// Thread-safe and non-blocking. | ||
| /// </summary> | ||
| internal sealed class TelemetryCollector : IDisposable |
There was a problem hiding this comment.
can we replace the methods with just one record method with an operation type?
| ``` | ||
|
|
||
| **Implementation Details**: | ||
|
|
There was a problem hiding this comment.
we don't need this level of detail in a design doc, in stead we should focus more on interface/contract between different class objects
There was a problem hiding this comment.
focus on adding more class diagram and sequence diagram, etc, instead of putting big block of code into the design doc.
| EnqueueEvent(telemetryEvent); | ||
| } | ||
|
|
||
| public void RecordStatementExecute( |
There was a problem hiding this comment.
what will happen, if data of same statement being reported multiple times, will our backend merge them?
| ``` | ||
|
|
||
| ### 9.3 Data Residency Compliance | ||
|
|
There was a problem hiding this comment.
do we need this section?
|
|
||
| #### 11.1.1 TelemetryCollector Tests | ||
|
|
||
| **File**: `TelemetryCollectorTests.cs` |
There was a problem hiding this comment.
maybe just a list of test case name, no need for detail code.
| } | ||
| ``` | ||
|
|
||
| ### 11.4 Mock Endpoint Testing |
|
|
||
| ## 12. Migration & Rollout | ||
|
|
||
| ### 12.1 Rollout Phases |
There was a problem hiding this comment.
let's remove this section
| ``` | ||
| ``` | ||
|
|
||
| #### Internal Documentation |
| @@ -0,0 +1,11 @@ | |||
| 1. "can you understand the content present in this google doc: {telemetry-design-doc-url}" | |||
There was a problem hiding this comment.
How do you plugin the actual url? Is this a databricks internal google doc link?
| @@ -0,0 +1,11 @@ | |||
| 1. "can you understand the content present in this google doc: {telemetry-design-doc-url}" | |||
|
|
|||
| 2. "can you use google mcp" | |||
There was a problem hiding this comment.
This is internal google doc mcp right?
| ```csharp | ||
| private async Task DownloadFileAsync(IDownloadResult downloadResult, ...) | ||
| { | ||
| var sw = Stopwatch.StartNew(); |
There was a problem hiding this comment.
Why do we need a separate stopwatch? The existing cloudfetch telemetry already capture this using ActivityTrace.
|
|
||
| 4. "can you check the databricks jdbc repo and understand how it is currently implemented" | ||
|
|
||
| 5. "now, lets go through the arrow adbc driver for databricks present at {project-location}/arrow-adbc/csharp/src/Drivers/Databricks, and understand its flow" |
There was a problem hiding this comment.
Can we ask llm to also go through this PR of how existing tracing/exporter framework is implemented: #3315.
I believe this can potentially reduce this complexity of our design and we can reuse some of the existing components.
| { | ||
| Debug.WriteLine($"Failed to check telemetry feature flag: {ex.Message}"); | ||
| // Default to enabled if check fails | ||
| return true; |
There was a problem hiding this comment.
Default to enable? Is this expected? Any side-effect(perf) for this exporter?
4 participants
Low level design doc for telemetry in databricks ADBC driver