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

Add trace span for time spent lazily loading index-header in store-gateway #6922

Merged
merged 4 commits into from
Dec 14, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Dec 13, 2023

What this PR does

This PR adds further information to trace spans emitted by store-gateways while lazily loading an index-header.

I'd suggest reviewing the commits separately - the first introduces a context.Context argument to the methods on the indexheader.Reader interface, and the second adds the trace span.

Which issue(s) this PR fixes or relates to

(none)

Checklist

  • [n/a] Tests updated.
  • [n/a] Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [n/a] about-versioning.md updated with experimental features.

@charleskorn charleskorn marked this pull request as ready for review December 13, 2023 06:33
@charleskorn charleskorn requested a review from a team as a code owner December 13, 2023 06:33
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

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

LGTM. My only concern is the number of spans we're going to be adding to a typical request since we're already running into trace size limits in some cases. I suppose this shouldn't be too bad in practice since we only add a span when doing lazy loading which is infrequent. WDYT?

@charleskorn
Copy link
Contributor Author

I suppose this shouldn't be too bad in practice since we only add a span when doing lazy loading which is infrequent. WDYT?

That was my assumption too, so I'm not expecting this to be particularly impactful.

@charleskorn charleskorn enabled auto-merge (squash) December 14, 2023 00:20
@charleskorn charleskorn merged commit 8bb9ea3 into main Dec 14, 2023
28 checks passed
@charleskorn charleskorn deleted the charleskorn/store-gateway-tracing branch December 14, 2023 00:34
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

before the store-gateway starts serving a request it creates an index header reader. This happens here

// Call IndexVersion to lazy load the index header if it lazy-loaded.
_, _ = b.indexHeaderReader.IndexVersion()

the idea was to trigger a method of the index header reader that will load it before starting to use it for general-purpose operations.

I suspect that this is the only place which may load an index header and we might not need all these extra context args.

@charleskorn
Copy link
Contributor Author

Sorry, might have been a little too enthusiastic merging this before you had a chance to review it @dimitarvdimitrov. Would you like me to revert this and explore an alternative approach?

@dimitarvdimitrov
Copy link
Contributor

I think that would keep the interfaces slightly simpler and still achieve the same. Sorry for not reviewing it on the day.

charleskorn added a commit that referenced this pull request Jan 8, 2024
@charleskorn charleskorn mentioned this pull request Jan 8, 2024
1 task
@charleskorn
Copy link
Contributor Author

I've reverted this change in #7065 - would you be able to take a look @dimitarvdimitrov?

charleskorn added a commit that referenced this pull request Jan 8, 2024
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.

None yet

4 participants