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

Write query parameters to span attributes instead of events #7046

Merged
merged 4 commits into from
Jan 20, 2024

Conversation

chencs
Copy link
Contributor

@chencs chencs commented Jan 4, 2024

What this PR does

Currently, it's next to impossible to use Tempo's TraceQL to search for traces on specific queries, as TraceQL doesn't support searching on event attributes. Here is a related comment pointing out this pain for Mimir.

Since query params aren't really an event in time, and therefore don't benefit from the timestamp that span events/logs require, I propose that instead of logging query parameters as span events, we store them as span attributes (a.k.a. "tags"). This will make traces searchable by query information (e.g., query string and step size) via TraceQL, and removes some span logs whose timing has no significance.

Which issue(s) this PR fixes or relates to

Relates to grafana/tempo#2313

Checklist

  • [N/A] Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [N/A] about-versioning.md updated with experimental features.

Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I only left a suggestion to rename the method which does this

pkg/frontend/querymiddleware/model_extra.go Outdated Show resolved Hide resolved
@chencs chencs force-pushed the casie/write-query-data-to-span-tags branch from e52e325 to 6036e0f Compare January 19, 2024 01:07
@chencs chencs marked this pull request as ready for review January 19, 2024 01:11
@chencs chencs requested a review from a team as a code owner January 19, 2024 01:11
@chencs chencs force-pushed the casie/write-query-data-to-span-tags branch from d671443 to ed25794 Compare January 19, 2024 01:35
Copy link
Contributor

@colega colega left a comment

Choose a reason for hiding this comment

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

Cool! Thanks!

@chencs chencs force-pushed the casie/write-query-data-to-span-tags branch from ed25794 to 0d3eb1f Compare January 20, 2024 00:00
@jhalterman jhalterman enabled auto-merge (squash) January 20, 2024 00:18
@chencs chencs force-pushed the casie/write-query-data-to-span-tags branch from 0d3eb1f to 439a0a2 Compare January 20, 2024 00:31
@jhalterman jhalterman enabled auto-merge (squash) January 20, 2024 00:43
@jhalterman jhalterman merged commit 1780c50 into main Jan 20, 2024
28 checks passed
@jhalterman jhalterman deleted the casie/write-query-data-to-span-tags branch January 20, 2024 00:45
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.

None yet

4 participants