Skip to content

Tempo: Refactor Trace By ID and Metrics Summary - #3522

Merged
joe-elliott merged 22 commits into
grafana:mainfrom
joe-elliott:refactor-final-endpoints
Apr 11, 2024
Merged

Tempo: Refactor Trace By ID and Metrics Summary#3522
joe-elliott merged 22 commits into
grafana:mainfrom
joe-elliott:refactor-final-endpoints

Conversation

@joe-elliott

@joe-elliott joe-elliott commented Mar 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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]
@joe-elliott
extend buildUpstreamRequest
638c263 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
metrics summary
5bf1ae0 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
trace by id first pass
31e1c77 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
fixed inf loop
440d8af 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
works
6ce0c4b 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
removed trace id hedging in the frontend
741b93e 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
todos, tests and cleanup
33f2830 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
add 429 test
d33271e 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
changelog
5dff65d 
Signed-off-by: Joe Elliott <number101010@gmail.com>
knylander-grafana
knylander-grafana reviewed Mar 26, 2024
View reviewed changes

@knylander-grafana knylander-grafana left a comment

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.

Doc updates look good. Thank you for updating the docs.
@joe-elliott
lint
5c758eb 
Signed-off-by: Joe Elliott <number101010@gmail.com>
electron0zero
electron0zero reviewed Mar 27, 2024
View reviewed changes
Comment thread docs/sources/tempo/configuration/manifest.md
@joe-elliott
restore api prefix
ff22035 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
pass the right param
f39d824 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
fix race in test
74b843b 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Merge branch 'main' into refactor-final-endpoints
2b2310e
@joe-elliott
move deduper into combiner
ad57206 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Move content type marshalling into combiner
a5bfcb6 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Merge branch 'main' into refactor-final-endpoints
998f635
@joe-elliott
lint: check err in test
1d53ed8 
Signed-off-by: Joe Elliott <number101010@gmail.com>
yvrhdn
yvrhdn reviewed Apr 11, 2024
View reviewed changes

@yvrhdn yvrhdn left a comment

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.

LGTM overall 👍 Left some minor comments
Comment thread integration/e2e/limits_test.go Outdated
Comment thread modules/frontend/combiner/interface.go Outdated
Comment thread modules/frontend/combiner/trace_by_id_deduper.go
Comment thread modules/frontend/combiner/trace_by_id_deduper.go Outdated
Comment thread modules/frontend/frontend.go Outdated
Comment thread modules/frontend/traceid_handlers.go Outdated
yvrhdn
yvrhdn approved these changes Apr 11, 2024
View reviewed changes

@yvrhdn yvrhdn left a comment

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.

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
@joe-elliott @yvrhdn
Update integration/e2e/limits_test.go
6445fde 
Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>
@joe-elliott @yvrhdn
Apply suggestions from code review
3053b1f 
Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>
electron0zero
electron0zero reviewed Apr 11, 2024
View reviewed changes
Comment thread modules/frontend/tenant.go
@joe-elliott
rename func for clarity
62d6fe2 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
review
3e1ce06 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
joe-elliott merged commit 475312b into grafana:main Apr 11, 2024
@electron0zero electron0zero mentioned this pull request Apr 15, 2024
Merged
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
@joe-elliott @yvrhdn
Tempo: Refactor Trace By ID and Metrics Summary (grafana#3522)
2618ed6 
* extend buildUpstreamRequest

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

* metrics summary

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

* trace by id first pass

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

* fixed inf loop

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

* works

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

* removed trace id hedging in the frontend

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

* todos, tests and cleanup

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

* add 429 test

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

* changelog

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

* lint

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

* restore api prefix

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

* pass the right param

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

* fix race in test

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

* move deduper into combiner

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

* Move content type marshalling into combiner

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

* lint: check err in test

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

* Update integration/e2e/limits_test.go

Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>

* Apply suggestions from code review

Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>

* rename func for clarity

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

* review

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

---------

Signed-off-by: Joe Elliott <number101010@gmail.com>
Co-authored-by: Koenraad Verheyden <koenraad.verheyden@posteo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@joe-elliott @yvrhdn @electron0zero @knylander-grafana