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

Tempo: Refactor Trace By ID and Metrics Summary #3522

Merged
merged 22 commits into from
Apr 11, 2024

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Mar 26, 2024

What this PR does:
Refactors 2 more endpoints to use the new frontend pipeline items. The final one will be the new query range endpoint. Trace by ID was more difficult than expected. It has a few unique features that make it harder to work with:

  • Passes proto instead of json
  • Changes the encoded object. All other paths propagate the same object from querier to outside
  • Has a special middleware that occurs after recombination (deduper)
  • Does not consider 404s an error

Testing is looking good for no regressions.

Other changes:

  • Removes trace by id hedging. We have found it to be an antipattern.
  • Fixes 429s being returned as 499s and adds an integration test
  • Removes the addition of a tenant in multitenant trace id lookup. We are not using it and it's difficult to fit into the new pipeline. Ignoring for now.
  • Reduce memory consumption in the frontend on trace by id lookup by pushing the deduper and content-type marshalling logic into the combiner. This prevents an unnecessary 2x marshalling/unmarshalling of a trace.

Which issue(s) this PR fixes:
Works toward #3413

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Doc updates look good. Thank you for updating the docs.

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Member

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

LGTM overall 👍 Left some minor comments

integration/e2e/limits_test.go Outdated Show resolved Hide resolved
modules/frontend/combiner/interface.go Outdated Show resolved Hide resolved
modules/frontend/combiner/trace_by_id_deduper.go Outdated Show resolved Hide resolved
modules/frontend/frontend.go Outdated Show resolved Hide resolved
modules/frontend/traceid_handlers.go Outdated Show resolved Hide resolved
Copy link
Member

@yvrhdn yvrhdn left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this 👍 just got some minor questions/issues, nothing blocking

joe-elliott and others added 2 commits April 11, 2024 11:58
Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>
Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott merged commit 475312b into grafana:main Apr 11, 2024
15 checks passed
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.

4 participants