Skip to content

Conversation

@msrathore-db
Copy link
Contributor

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

Summary

Adds comprehensive Activity-based logging to DatabricksStatement.cs for improved observability and debugging of Databricks ADBC operations.

Changes

Methods with Logging Added

  • SetStatementProperties: Logs configuration (Arrow native types, CloudFetch settings, async mode)
  • GetSchemaFromMetadata: Logs schema parsing decisions (Arrow vs Thrift, column count)
  • GetCatalogsAsync: Logs catalog queries with feature flags and row counts
  • GetSchemasAsync: Logs schema queries with catalog filtering
  • GetTablesAsync: Logs table queries with SPARK catalog handling
  • GetColumnsAsync: Logs column queries with search criteria
  • GetPrimaryKeysAsync: Logs primary key queries
  • GetCrossReferenceAsync: Logs foreign key queries
  • GetColumnsExtendedAsync: Logs DESC TABLE EXTENDED queries
  • SetOption: Logs configuration changes

What Gets Logged

Tags:

  • Statement configuration (CloudFetch settings, Arrow native types, batch size)
  • Feature flags (enable_multiple_catalog_support, pk_fk_enabled)
  • Query parameters (catalog, schema, table, column names)
  • Results (db.response.returned_rows)

Events:

  • statement.<operation>.start / statement.<operation>.complete
  • Decision points (calling_base_implementation, returning_empty_result, fallback_to_base)
  • Schema handling (using_arrow_schema, fallback_to_thrift)

Example Log Output

{
  "OperationName": "GetCatalogs",
  "Duration": "00:00:00.582",
  "TagObjects": {
    "statement.feature.enable_multiple_catalog_support": true,
    "db.response.returned_rows": 28
  },
  "Events": [
    { "Name": "statement.get_catalogs.start" },
    { "Name": "statement.get_catalogs.calling_base_implementation" },
    { "Name": "statement.get_catalogs.complete" }
  ]
}

Testing

Tested locally by enabling logging with:

properties["adbc.traces.exporter"] = "adbcfile";

Verified that all tags, events, and distributed tracing context are captured correctly in trace files for all implemented methods (GetCatalogs, GetSchemas, GetTables, GetColumns, and statement configuration).

PR generated by Cursor.

@github-actions github-actions bot added this to the ADBC Libraries 21 milestone Oct 25, 2025
@msrathore-db msrathore-db marked this pull request as draft October 25, 2025 13:41
@msrathore-db msrathore-db marked this pull request as ready for review October 25, 2025 13:47
protected override Schema GetSchemaFromMetadata(TGetResultSetMetadataResp metadata)
{
// Log schema parsing decision
Activity.Current?.SetTag("statement.schema.has_arrow_schema", metadata.__isset.arrowSchema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this output logs correctly? Why not using this.TraceActivity?
@birschick-bq What should be the recommended way for trace?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was concerned that Activity.Current might not be set to the activity in your current block - i.e., it might have been set for a child call. But in thinking about it, I don't think that would be the case, as long as the child call disposed the Activity in their scope.

However, I think the best usage pattern for Activity.Current is where you don't start a new activity and you don't pass an existing activity as a parameter. That is, a situation in which you might be called from a method that may or may not have a current activity started.

So I think Activity.Current is used appropriately in the case shown in line 93, above.

Use this.TraceActivity when you want a new activity (line) with its own events and tags. Typically something "major" or structural in your code. If the new activity (this.TraceActivity) is a child call, the ParentId will be set to indicate it is a child activity of the parent activity.

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.

3 participants