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

db: improve context propagation for tracing of slow block loads #3728

Open
3 of 4 tasks
sumeerbhola opened this issue Jul 2, 2024 · 1 comment
Open
3 of 4 tasks
Assignees
Labels
A-storage O-support P-3 Issues/test failures with no fix SLA T-storage

Comments

@sumeerbhola
Copy link
Collaborator

sumeerbhola commented Jul 2, 2024

We sometimes see traces in CockroachDB which suggest slowness in Pebble, but the InternalIteratorStats printed don't indicate block cache misses. This is possibly because those stats are incomplete (don't include Reader creation or loading of various block types). We have a LoggerAndTracer passed to Pebble, and tracing of slow block loads (5ms is the slow threshold in Reader.readBlock), since #2055. But there are some significant gaps:

  • Many Reader.readBlock calls are using a background context, e.g. Reader.readRangeDel, readRangeKey, readMetaindex. These should use a real context.
  • tableCacheValue.load which creates a new Reader should accept a context. We have one in the callpath that originates in tableCacheShard.newIters.

Jira issue: PEBBLE-36

[Radu] Making a checklist for context propagation:

  • tableCacheValue.load (in the newIters path)
  • Ingest methods
  • Value block load
  • Reader block reads (readRangeDel, readRangeKey, readMetaindex)
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 8, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 8, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 8, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
@nicktrav nicktrav moved this from Incoming to Next in [Deprecated] Storage Jul 9, 2024
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Jul 10, 2024
126824: storage: make pebbleLogger.Eventf also log r=RaduBerinde,jbowens a=sumeerbhola

This is useful when tracing is not enabled, but verbosity of logging can be increased. Also, we currently have gaps in context propagation in tracing which this can help mitigate for scenarios where slowness is reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None

Co-authored-by: sumeerbhola <[email protected]>
blathers-crl bot pushed a commit to cockroachdb/cockroach that referenced this issue Jul 10, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
asg0451 pushed a commit to asg0451/cockroach that referenced this issue Jul 10, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Jul 11, 2024
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Jul 12, 2024
RaduBerinde added a commit to RaduBerinde/pebble that referenced this issue Jul 12, 2024
This change plumbs a context to `NewReader` covering the most
important `newIters` code path. This context is/will be used to
trace slow footer and metaindex reads.

Informs cockroachdb#3728
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 12, 2024
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Jul 12, 2024
sumeerbhola added a commit to sumeerbhola/pebble that referenced this issue Jul 12, 2024
sumeerbhola added a commit that referenced this issue Jul 12, 2024
sumeerbhola added a commit that referenced this issue Jul 12, 2024
RaduBerinde added a commit to RaduBerinde/pebble that referenced this issue Jul 13, 2024
This change plumbs a context to `NewReader` covering the most
important `newIters` code path. This context is/will be used to
trace slow footer and metaindex reads.

Informs cockroachdb#3728
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 13, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Jul 13, 2024
This is useful when tracing is not enabled, but verbosity of logging can be increased. Also, we currently have gaps in context propagation in tracing which this can help mitigate for scenarios where slowness is reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
sumeerbhola added a commit to cockroachdb/cockroach that referenced this issue Jul 13, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is
reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
nicktrav pushed a commit to cockroachdb/cockroach that referenced this issue Jul 15, 2024
This is useful when tracing is not enabled, but verbosity of logging can
be increased. Also, we currently have gaps in context propagation in
tracing which this can help mitigate for scenarios where slowness is reproducible.

Relates to cockroachdb/pebble#3728

Epic: none

Release note: None
RaduBerinde added a commit to RaduBerinde/pebble that referenced this issue Jul 17, 2024
This change plumbs a context to `NewReader` covering the most
important `newIters` code path. This context is/will be used to
trace slow footer and metaindex reads.

Informs cockroachdb#3728
RaduBerinde added a commit that referenced this issue Jul 17, 2024
This change plumbs a context to `NewReader` covering the most
important `newIters` code path. This context is/will be used to
trace slow footer and metaindex reads.

Informs #3728
RaduBerinde added a commit to RaduBerinde/pebble that referenced this issue Jul 17, 2024
This context covers the read of the range del or range key block.

Informs cockroachdb#3728
RaduBerinde pushed a commit to RaduBerinde/pebble that referenced this issue Jul 17, 2024
RaduBerinde added a commit that referenced this issue Jul 20, 2024
This context covers the read of the range del or range key block.

Informs #3728
@nicktrav nicktrav added the P-2 Issues/test failures with a fix SLA of 3 months label Jul 31, 2024
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Aug 1, 2024
Relates to cockroachdb/pebble#3728

Epic: none

Release note (ops change): Verbose logging of slow pebble reads is no
longer enabled via --vmodule=pebble_logger_and_tracer=2, where
pebble_logger_and_tracer contains the CockroachDB implementation of the
logger needed by Pebble, and instead requires listing the Pebble files
containing the log statements, specifically --vmodule=reader=2,table=2.
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Aug 1, 2024
127066: log,storage: add ExpensiveLogEnabledVDepth for use in pebbleLogger r=RaduBerinde,arjunmahishi a=sumeerbhola

Relates to cockroachdb/pebble#3728

Epic: none

Release note (ops change): Verbose logging of slow pebble reads is no
longer enabled via --vmodule=pebble_logger_and_tracer=2, where
pebble_logger_and_tracer contains the CockroachDB implementation of the
logger needed by Pebble, and instead requires listing the Pebble files
containing the log statements, specifically --vmodule=reader=2,table=2.

Co-authored-by: sumeerbhola <[email protected]>
RaduBerinde added a commit to RaduBerinde/pebble that referenced this issue Aug 1, 2024
RaduBerinde added a commit that referenced this issue Aug 13, 2024
@RaduBerinde RaduBerinde removed the P-2 Issues/test failures with a fix SLA of 3 months label Oct 15, 2024
@RaduBerinde
Copy link
Member

Removing the P-2 label because the important paths have been addressed. There is only one path around value blocks that is more tricky.

@RaduBerinde RaduBerinde added the P-3 Issues/test failures with no fix SLA label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage O-support P-3 Issues/test failures with no fix SLA T-storage
Projects
No open projects
Development

No branches or pull requests

3 participants