microsoft / microsoft/mssql-rs

Remove per-row tracing overhead (`#[instrument]` + hot-path logs) (row decode perf, axis: precomputation)

Open Beginner friendly
#256 1 comment 0 reactions 0 assignees View on GitHub
enhancement performance
Dominant language
Rust
Stars
53
Forks
14
Avg merge
1d 15h
Merged PRs (30d)
137

Description

Sub-issue of #247 (axis: **Precomputation**).

### Problem statement

`#[instrument]` on an `async fn` wraps the returned future in `Instrumented`. That has two costs beyond log formatting:

1. It **enlarges the async state machine**.
2. It **enters and exits a span on every `poll`** — not once per call.

On a per-row entry point that is millions of span transitions per scan. This matters because it means **level filtering alone does not help** — the span machinery runs regardless of whether any subscriber is listening. The attribute itself has to go.

The per-row and per-token `info!` / `debug!` statements are a smaller, separate cost.

### Proposed solution

1. Remove `#[instrument]` from per-row entry points.
2. **Downgrade** the per-row/per-token log statements to `trace!` rather than deleting them.

**Feasibility: ✅ proven.** Prototyped in `56a836f6`, green on all six configs in #247.

#### Findings

- **There are 5 sites, not the 3 the deck claims:**

| Site | What |
|---|---|
| `tds_client.rs:3076` | `#[instrument]` |
| `tds_client.rs:3943` | `#[instrument]` |
| `tds_client.rs:3133` | `info!("Row Received")` |
| `tds_client.rs:3400` | `info!("Row Received")` |
| `token_stream.rs:371` | `debug!("Parsing token type")` |
| `token_stream.rs:597` | `debug!("Parsing token type")` |

- **PR #238 deletes these outright. Recommend downgrading instead.** Moving `info!`/`debug!` → `trace!` keeps the diagnostics available under `RUST_LOG=trace` at effectively zero cost when disabled. Losing per-token tracing would be a real debuggability regression for protocol work, and it is the first thing anyone reaches for when debugging a malformed token stream.

- Only the `#[instrument]` attributes must be removed outright, since their cost is not level-gated.

### Affected crate

mssql-tds

### Alternatives considered

**Keep `#[instrument]` and rely on level filtering.** Does not work — see above. `Instrumented` costs are paid at poll time regardless of subscriber state.

**Delete the log statements entirely (what #238 does).** Rejected — the cost of a disabled `trace!` is negligible, and per-token visibility is genuinely useful.

### Additional context

This is the lowest-risk item in #247 and a reasonable first PR for whoever picks up this work.

Nothing here is benchmarked. The deck attributes 0.390s to this change; confirm independently.

Contributor guide

Open the contributing guide

Research direction

Start with the five listed sites in mssql-tds: the two #[instrument] attributes and per-row logs in tds_client.rs, plus per-token logs in token_stream.rs. Review the prototype commit 56a836f6 and the six configurations referenced in #247; done means the attributes are removed, the logs are trace-level, and the existing configurations remain green.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
databases, performance
Issue type
Refactor
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.