Skip to content
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

sql, tracing: refactor statement span creation for the instrumentation helper #85820

Open
THardy98 opened this issue Aug 9, 2022 · 0 comments
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team

Comments

@THardy98
Copy link

THardy98 commented Aug 9, 2022

We would like to refactor statement span creation for the instrumentation helper to provide guarantees with the contents of the span field in the instrumentation helper (the ih.sp field). In particular, we would like the contents of the instrumentation helper's span to be known when used to compute query-level statistics.

This issue spawned from discussion on #84718, where the following concerns/suggestions were made:

GetQueryLevelStats seems to make assumptions about what it finds in the trace. Do those assumptions hold when the ih did not create the span? I think they do, because the span in question is still limited to the current statement (but please check). But this all seems very shaky; if it wasn't for that span creation for the statement, it would break (I think).

I think we should either a) have the ih always create a span, or b) never create a span - and rely and assert that there is a statement span.
I think a) is easier but b) is nicer.

For b), I think we need to:

  1. add the WithForceRealSpan option to the statement span creation, in order to assure that the span is created even when trace.span_registry.enabled cluster setting is tuned off (the default is on).
  2. in the ih, assert that we're operating on a statement span, by checking the span's operation name
  3. change the tracing library so that children of a "real span" are not automatically forced to be real spans themselves. The relevant check is the opts.parentTraceID at span creation time. I was surprised that the check is there; I don't think it's intentional. I don't think that the parent being a real span is reason enough for the child to be a real span. Valid reasons for the child being a real span include the active spans registry being enabled enabled (which is the default), or that the parent recording at the time the child is being created.

Without 3), bullet number 1) would effectively neuter the trace.span_registry.enabled = off setting because, by creating a real span at such a high level, we'd essentially be creating spans everywhere.

Jira issue: CRDB-18450

@THardy98 THardy98 added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-observability T-sql-queries SQL Queries Team labels Aug 9, 2022
@mgartner mgartner moved this to Backlog (DO NOT ADD NEW ISSUES) in SQL Queries Jul 24, 2023
@mgartner mgartner moved this from Backlog (DO NOT ADD NEW ISSUES) to New Backlog in SQL Queries Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-queries SQL Queries Team
Projects
Status: Backlog
Development

No branches or pull requests

2 participants