Handle prefixes when listing blocks from S3 and GCS

[bpfoster]

What this PR does: Adjusts the logic to list blocks from S3 and GCS to consider object path prefix.
You can see that tempodb/backend/s3/s3.go would ignore any results whose key, upon splitting by /, did not have 3 elements. When a prefix is configured, e.g. a/b/c/, the returned key will instead be something like a/b/c/<tenantID>/<blockID>/meta. Stip the prefix off this path before pulling out those 3 useful bits from the path.

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

Checklist

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

[CLAassistant]

All committers have signed the CLA.

[joe-elliott]

ok, this is undoubtedly related to the polling improvements we made in 2.4.0. thx for looking at this. it is likely broken in azure/gcs as well.

@zalegrala can you review?

[bpfoster]

Yes it looks like you were right that it was introduced in 2.4.0. Running 2.3.1 looks like the compactor is cleaning up as expected.

Though I don't have Azure or GCS accounts to do integration testing, I have added to this PR unit tests covering these cases for both that are similar to S3. As you expected, the issue is present in the GCS implementation (and I have added a fix for it to this PR), but looks like the Azure implementations already are correctly stripping the prefix.

[zalegrala]

Thank you for the PR, and for fixing all the backends. It might be nice to have an e2e test for this, but it digging in there is a little more work. If you're willing that would be great, but if its too much, I won't push. This is already a good step.

Part of the testing for the work that you are fixing was integrated into a test here to specifically test the poller with the various backends. It might be simple enough to update the config to include a prefix. We'll want to test with and without a prefix.

https://github.com/grafana/tempo/blob/main/integration/poller/poller_test.go#L45

Alternatively we have the main e2e tests that could be adjusted to include prefix testing.

https://github.com/grafana/tempo/blob/main/integration/e2e/e2e_test.go#L42

Probably duplicating the above with a config change would work.

[bpfoster]

Thanks @zalegrala . I'll address your comments and then take a look at the integrations tests and see if I can make something happen there.

[bpfoster]

@zalegrala - please see the new commits when you have a chance.

• Addressed your comment with the unit test regarding checking block IDs
• Updated both the poller_test and e2e_test to exercise configuration with and without prefixes. Feel free to choose both/either/neither :) There are also other ways we could handle the test permutations, so let me know if you're not a fan of how I incorporated those changes into the tests.

Both the integrations tests confirm that S3 and GCS backends fail when configured with a prefix in v2.4.0 ¹.
Both confirm that Azure is in fact unaffected.
Both confirm that S3 and GCS are fixed in this PR.

I did slightly refactor my changes to the S3 and GCS backends to be more in line with the Azure backends. This has the added benefit over the prior iteration of working when the configured prefix does not end in a /. This is covered here in prefix test cases in both integration tests.

Given that Azure has been verified in the integration tests, I believe we can resolve the discussion on CHANGELOG.md?

Footnotes

1. I don't actually have a hard failure in the e2e test against v2.4.0. I gave up waiting on the test to complete after 30 minutes (stuck on the tempo.WaitSumMetricsWithOptions() line). I believe that the failure to complete, along with logs always showing metas=0, is a symptom of the issue - no blocks ever returned. ↩

[zalegrala]

Thank you for the updates, I've approved the CI. I think the only concern I have is the one you mention, which is the amount of time it takes for the e2e tests. The change to the poller_test looks good and I appreciate all the permutations, but perhaps we reduce the permutations on the e2e test as a matter of pragmatism. The e2e tests take much longer to run generally.

I appreciate you confirming the Azure code with this test also. Yes, I believe you are correct about the CHANGELOG. I checked the code of both azure versions and it looks like this case is handled, so just the two you are fixing were missed.

I see a couple failures, but I'll check out the code and run locally to take a look. Let's see what the rest of the CI checks look like.

[bpfoster]

I addressed the format error, fixed the failing e2e tests, and removed 2/4 of the prefix test case permutations from the e2e test.

It looks like all these are passing the CI checks now. The lint job is failing in the last run, but this looks more like a CI job issue, I don't think any changes here would cause the errors being reported.

A local run of golangci-lint run --new-from-rev=origin/main reports 1 error but it is from existing code that just had indentation changed.

integration/e2e/e2e_test.go:113:50: string `/api/echo` has 5 occurrences, make it a constant (goconst)
				assertEcho(t, "http://"+tempo.Endpoint(3200)+"/api/echo")

[zalegrala]

This looks good to me. Thank you for the contribution.

[github-actions]

The backport to release-v2.4 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-3466-to-release-v2.4 origin/release-v2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 8e6e7fe86f79d1e02972d0d01b35174d25237f81

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-3466-to-release-v2.4
# Create the PR body template
PR_BODY=$(gh pr view 3466 --json body --template 'Backport 8e6e7fe86f79d1e02972d0d01b35174d25237f81 from #3466{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "[release-v2.4] Handle prefixes when listing blocks from S3 and GCS" --body-file - --label "type/bug" --label "backport" --base release-v2.4 --milestone release-v2.4 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-3466-to-release-v2.4

# Create a pull request where the `base` branch is `release-v2.4` and the `compare`/`head` branch is `backport-3466-to-release-v2.4`.

# Remove the local backport branch
git switch main
git branch -D backport-3466-to-release-v2.4