Skip to content

Conversation

@msrathore-db
Copy link
Contributor

@msrathore-db msrathore-db commented Oct 26, 2025

Summary

Adds Activity-based logging to RetryHttpHandler.cs for improved observability of HTTP retry behavior.

Changes

Architecture: Activity Propagation

The handler implements the IActivityTracer pattern for proper trace context propagation:

  • Implements IActivityTracer interface - Delegates trace operations to the connection
  • Accepts IActivityTracer parameter - Connection instance passed in constructor
  • Uses TraceActivityAsync wrapper - Creates Activities properly for telemetry listeners
  • Propagates to clients - Works in PowerBI Desktop and other downstream consumers

This ensures Activity context flows correctly through the HTTP handler chain, following the same pattern as CloudFetchDownloader.

Logging Strategy

  • No logging for successful first-attempt requests (most common case ~99%)
  • Per-attempt logging when retries occur
  • Exception logging with context for failures

What Gets Logged

Tags (When Retries Occur):

  1. http.retry.attempt - Current attempt number (logged per retry)
  2. http.retry.total_attempts - Total number of retry attempts
  3. http.response.status_code - HTTP status code
  4. http.retry.outcome - Only for failures: cancelled or timeout_exceeded

Exception Context (On Failures):

  • error.context - Failure reason (http.retry.cancelled or http.retry.timeout_exceeded)
  • attempts - Number of attempts made
  • total_retry_seconds / timeout_seconds - Timing information (timeout cases only)

Retryable Status Codes: 408, 502, 503, 504

Example Log Output

Success (No Retries):

// NO LOGGING - Most efficient path

Success After Retries:

{
  "Tags": {
    "http.retry.attempt": 1,
    "http.response.status_code": 503,
    "http.retry.attempt": 2,
    "http.response.status_code": 503,
    "http.retry.total_attempts": 2,
    "http.response.status_code": 200
  }
}

Timeout Exceeded:

{
  "Tags": {
    "http.retry.total_attempts": 3,
    "http.response.status_code": 503,
    "http.retry.outcome": "timeout_exceeded"
  },
  "Exception": {
    "error.context": "http.retry.timeout_exceeded",
    "attempts": 3,
    "total_retry_seconds": 7,
    "timeout_seconds": 30
  }
}

Testing

  • ✅ All 14 unit tests passing
  • ✅ Activity propagation verified via IActivityTracer implementation
  • ✅ Follows same pattern as CloudFetchDownloader (proven in production)

Breaking Changes

None. Additive logging only.

@github-actions github-actions bot added this to the ADBC Libraries 21 milestone Oct 26, 2025
@msrathore-db msrathore-db changed the title feat(csharp/src/Drivers/Databricks): Add ultra-minimal Activity-based logging to RetryHttpHandler feat(csharp/src/Drivers/Databricks): Add minimal Activity-based logging to RetryHttpHandler Oct 26, 2025
Copy link
Contributor

@eric-wang-1990 eric-wang-1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this E2E with PowerBI?
I am actually not sure if this can be logged correctly, I wonder if you need to do similar things of CloudFetchDownlader which is to propagate the top level Trace function:

ActivityTrace IActivityTracer.Trace => _statement.Trace;
.

Can you test it with latest PBI desktop and see if you can see your logs.

msrathore-db and others added 2 commits November 5, 2025 05:42
Address reviewer feedback by implementing proper Activity tracing
infrastructure following the CloudFetchDownloader pattern.

Changes:
- RetryHttpHandler now implements IActivityTracer interface
- Accepts IActivityTracer parameter to delegate to connection's trace
- Wraps retry logic in TraceActivityAsync for proper Activity creation
- DatabricksConnection passes 'this' to RetryHttpHandler constructor
- Updated all unit tests with MockActivityTracer

This ensures Activity-based logging works reliably in client scenarios
like PowerBI Desktop by properly propagating trace context through the
handler chain, rather than relying on Activity.Current which may be null.

Co-Authored-By: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants