Skip to content
This repository was archived by the owner on Jul 28, 2026. It is now read-only.

Split trace agreggation - #743

Closed
mapno wants to merge 8 commits into
grafana-cold-storage:mainfrom
mapno:split-trace-agreggation
Closed

Split trace agreggation#743
mapno wants to merge 8 commits into
grafana-cold-storage:mainfrom
mapno:split-trace-agreggation

Conversation

@mapno

@mapno mapno commented Jul 15, 2021

Copy link
Copy Markdown
Contributor

PR Description

Split trace aggregation from tail sampling, as it can be useful for other purposes too, such as service graphs or other processors that benefit from having complete traces.

The pipeline will use groupbytrace processor for aggregation, except when using tail sampling, in which case grouping will be performed by the embedded aggregator in the sampling processor. It's not possible to have the tail sampling processor only sampling and leave aggregation to groupbytrace. Tail sampling has to buffer traces for at least 1s, so it's just better not to use groupbytrace in that case for simplicity and performance.

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@mapno
mapno requested a review from joe-elliott as a code owner July 15, 2021 10:01
@mapno mapno changed the title Split trace agreggation WIP: Split trace agreggation Jul 15, 2021
@mapno

mapno commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

There are a few things still missing from the PR, mostly documentation, so I marked it as a draft.

@mapno mapno changed the title WIP: Split trace agreggation Split trace agreggation Jul 15, 2021
@mapno

mapno commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

It's ready for review now

[ password: <secret> ]
[ password_file: <string> ]

# Configures aggregation of spans by trace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need better docs surrounding this. Perhaps we add some docs to the Tempo site and link them here? Things that need to be mentioned:

  1. Increased cpu/memory usage
  2. How rerouting the traces works (so teams can make sure that their network topologies are compatible)
  3. This only makes sense if you use tail sampling or service graphs.
  4. Explains the relationship between this and the load balancing processor.

#
# Longer waiting times will increase the number of traces that are correctly
# grouped. However, it will also increase the memory overhead of the processor.
group_by_trace:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't the "load balancing" settings be moved out as well? maybe renamed something to suggest it's routing by trace id.

[ wait: <duration> | default="5s" ]

# Configures the max amount of traces to keep in memory waiting for the duration
[ num_traces: <int> | default="1_000_000" ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

max_traces?

@mapno mapno closed this Jul 22, 2021
@mapno
mapno deleted the split-trace-agreggation branch July 22, 2021 09:17
@mapno mapno mentioned this pull request Jul 23, 2021
3 tasks
@github-actions github-actions Bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 11, 2024
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants