Frontend Refactor by joe-elliott · Pull Request #3400 · grafana/tempo

joe-elliott · 2024-02-15T20:32:05Z

What this PR does:
Refactors the frontend pipeline to accomplish a number of goals detailed below. Currently only the search pipelines have been moved over to the new structure. There will be follow up PRs for other endpoints.

Goals:

Provide streaming on all query endpoints
Consolidate recombination logic in the frontend/combiner package
Consolidate all pipeline building logic in the frontend/pipeline package

See the readme in the pipeline package for details.

Which issue(s) this PR fixes:
Fixes #1392

This PR is progress toward #3366 with an eventual goal to close it.

Changes

removes tempo_query_frontend_multitenant... metrics b/c they don't quite fit into the current pattern. this can be improved later.

todo:

Flesh out readme.md in /pipeline to describe how to build new pipeline elements

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>

knylander-grafana · 2024-02-16T03:13:32Z

@joe-elliott Was there doc added? I didn't see any in the file list to review.

joe-elliott · 2024-02-16T12:32:52Z

@joe-elliott Was there doc added? I didn't see any in the file list to review.

There was not. This change is invisible to the user. Unchecking the docs item!

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

mdisibio · 2024-02-23T15:21:24Z

+
+	go func() {
+		if resps != nil {
+			asyncResp.Send(resps)


Could you help me understand more about what resps is? My understanding is that asyncResp.Send(resp) above is the core data flow, sending things up the chain as they come in. This looks like a final send that's already known by the caller?

this was added to allow the sharder to craft responses unrelated to a request and send them back up the chain. in this case it's used to communicate search job size data up to the combiner.

created here:
https://github.com/grafana/tempo/pull/3400/files#diff-80f55bc33c867c54319fcdfef902b821f96f15e3ec594167bd4c9ab266b7b6eaR120

processed here:
https://github.com/grafana/tempo/pull/3400/files#diff-96cf01ab28f4274fe65e4501fbf82cd479266202d7bc766d3a7c720475568cdaR38-R41

this highlights the largest complication with this PR. code that used to do the recombining was highly context aware:
https://github.com/grafana/tempo/blob/main/modules/frontend/searchsharding.go#L245-L318

b/c it was just executed after all jobs had completed in the same function. Now we are pushing all jobs up to the collector level to stream results. As a result some of that context is lost and it creates "message passing" code like this.

Got it, thanks. Caching is another instance, where the cacheKey is passed through the context.

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

mdisibio · 2024-02-29T20:38:40Z

+
+	go func() {
+		if resps != nil {
+			asyncResp.Send(resps)


Got it, thanks. Caching is another instance, where the cacheKey is passed through the context.

mdisibio · 2024-02-29T20:49:40Z

+}
+
+// fetch fetches the response body from the cache. the caller assumes the responsibility of closing the response body.
+func (c *frontendCache) fetch(key string, pb proto.Message) bool {


This method looks unused except for the test, and wasn't sure about json+proto.Message. Since we know it's a proto.Message we could store the binary representation in cache instead.

yup. i do think there's an opportunity to make the sync part of the pipeline type aware with generics which would allow this, but we're not quite there yet

mdisibio

I really like this, the async parts seem clearer and streaming everywhere will be awesome. Definitely think we can make the sync parts (cache, etc) also async in the future.

* async handlers Signed-off-by: Joe Elliott <number101010@gmail.com> * search pipeline Signed-off-by: Joe Elliott <number101010@gmail.com> * cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * fixed api tests Signed-off-by: Joe Elliott <number101010@gmail.com> * make err last param - lint Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * remove test for dropped metrics Signed-off-by: Joe Elliott <number101010@gmail.com> * pipeline readme Signed-off-by: Joe Elliott <number101010@gmail.com> * cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * responses cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * review comments Signed-off-by: Joe Elliott <number101010@gmail.com> * remove unnecessary goroutine Signed-off-by: Joe Elliott <number101010@gmail.com> * async sharder takes len Signed-off-by: Joe Elliott <number101010@gmail.com> * nolint Signed-off-by: Joe Elliott <number101010@gmail.com> * AsyncSharder rename/cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * fix integration tests Signed-off-by: Joe Elliott <number101010@gmail.com> * lint >.< Signed-off-by: Joe Elliott <number101010@gmail.com> * remove special context handling in the grpc collector Signed-off-by: Joe Elliott <number101010@gmail.com> * prevent goroutine leak when sending to a full channel Signed-off-by: Joe Elliott <number101010@gmail.com> * buffer error to guarantee at least one makes it Signed-off-by: Joe Elliott <number101010@gmail.com> * responses in error should stay in error Signed-off-by: Joe Elliott <number101010@gmail.com> * test for returning the same error Signed-off-by: Joe Elliott <number101010@gmail.com> * don't pass requests in error forward Signed-off-by: Joe Elliott <number101010@gmail.com> * log http results even on err Signed-off-by: Joe Elliott <number101010@gmail.com> --------- Signed-off-by: Joe Elliott <number101010@gmail.com>

joe-elliott added 3 commits February 15, 2024 07:34

async handlers

9591c52

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

search pipeline

c8f47f5

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

cleanup

5883ab4

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

joe-elliott requested review from annanay25, electron0zero, ie-pham, mapno, mdisibio, stoewer, yvrhdn and zalegrala as code owners February 15, 2024 20:32

joe-elliott added 4 commits February 15, 2024 15:45

lint

37cb548

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

fixed api tests

753d959

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

make err last param - lint

42d22e9

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

lint

d924132

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

joe-elliott added 2 commits February 16, 2024 08:11

remove test for dropped metrics

4ae78ca

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

pipeline readme

82a2439

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

joe-elliott mentioned this pull request Feb 20, 2024

Grafana Cloud Tempo: Default Start Time view for traces in Tempo does not order by most recent #3109

Closed

Jxlam-pdx assigned joe-elliott Feb 20, 2024

cleanup

25fb5ad

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

joe-elliott mentioned this pull request Feb 20, 2024

Tempo Query Frontend Refactor #3413

Closed

9 tasks

joe-elliott added 2 commits February 20, 2024 14:41

Merge branch 'main' into frontend-o7-beta

e40432b

responses cleanup

2a8aff0

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

mdisibio reviewed Feb 23, 2024

View reviewed changes

Comment thread modules/frontend/pipeline/pipeline.go Outdated

Comment thread modules/frontend/pipeline/async_sharding.go

Comment thread modules/frontend/pipeline/async_sharding.go Outdated

mdisibio reviewed Feb 23, 2024

View reviewed changes

Comment thread modules/frontend/combiner/common.go Outdated

Comment thread modules/frontend/frontend.go

Comment thread modules/frontend/frontend.go Outdated

Comment thread modules/frontend/frontend.go

joe-elliott added 2 commits February 26, 2024 08:03

review comments

d2ec088

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

Merge branch 'main' into frontend-o7-beta

a1c8833

joe-elliott added 14 commits February 26, 2024 10:24

remove unnecessary goroutine

87edbca

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

async sharder takes len

3904855

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

nolint

78f71eb

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

AsyncSharder rename/cleanup

4b01755

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

lint

6676b4e

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

fix integration tests

8da6169

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

lint >.<

5140496

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

remove special context handling in the grpc collector

6c7eeaf

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

prevent goroutine leak when sending to a full channel

cf260d0

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

buffer error to guarantee at least one makes it

fd90fe0

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

responses in error should stay in error

2222855

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

test for returning the same error

752ad3d

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

don't pass requests in error forward

0589996

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

log http results even on err

7375e3b

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

joe-elliott requested a review from mdisibio February 28, 2024 14:21

mdisibio reviewed Feb 29, 2024

View reviewed changes

mdisibio approved these changes Feb 29, 2024

View reviewed changes

joe-elliott merged commit c889e63 into grafana:main Mar 1, 2024

joe-elliott mentioned this pull request Mar 4, 2024

Frontend Refactor: Streaming tags #3460

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Frontend Refactor - #3400

Frontend Refactor#3400
joe-elliott merged 28 commits into
grafana:mainfrom
joe-elliott:frontend-o7-beta

joe-elliott commented Feb 15, 2024 •

edited

Loading

knylander-grafana commented Feb 16, 2024

joe-elliott commented Feb 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdisibio Feb 23, 2024

joe-elliott Feb 26, 2024 •

edited

Loading

mdisibio Feb 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdisibio Feb 29, 2024

mdisibio Feb 29, 2024

joe-elliott Mar 1, 2024

mdisibio left a comment

Labels

3 participants

Uh oh!

Conversation

joe-elliott commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

knylander-grafana commented Feb 16, 2024

joe-elliott commented Feb 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdisibio Feb 23, 2024

Choose a reason for hiding this comment

joe-elliott Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mdisibio Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdisibio Feb 29, 2024

Choose a reason for hiding this comment

mdisibio Feb 29, 2024

Choose a reason for hiding this comment

joe-elliott Mar 1, 2024

Choose a reason for hiding this comment

mdisibio left a comment

Choose a reason for hiding this comment

Labels

3 participants

joe-elliott commented Feb 15, 2024 •

edited

Loading

joe-elliott Feb 26, 2024 •

edited

Loading