backupccl: add some span recordings to the backup resumer #65725

adityamaru · 2021-05-26T16:19:17Z

This change adds some initial span recordings to the backup
resumer that might be interesting when inspecting an executing
backup. The processor and KV level requests will be dealt
with in subsequent commits.

Informs: #64992

Release note: None

cockroach-teamcity · 2021-05-26T16:19:23Z

This change is

adityamaru · 2021-05-26T16:23:51Z

Just wanted to get some eyes on this general approach before I went any further. Primarily, if there are any better ideas of how we would like to organize trace recording protobufs.
I went with trying to split backup specific, and common protos that can be used by restore and import as well.

Also welcoming any high-level pointers on if we want to be more structured about where we want to add more recording. I've just used my own discretion at the moment to identify interesting phases in the resumer.

miretskiy · 2021-05-26T16:54:58Z

Are we trying to be too structured? For example:

resumerSpan.RecordStructured(&jobspb.BulkJobTraceEvent{Message: "writing backup manifest"})

We already have a way of tracing string values, don't we? Just thinking out loud: what are we getting for having
these strongly typed structured traces? Are we planning on doing some sort of analysis at some point (like how many traces have x.y > z? Or is more of a log message type (just strings, possibly formatted)?

adityamaru · 2021-05-26T17:12:46Z

We already have a way of tracing string values, don't we? Just thinking out loud: what are we getting for having
these strongly typed structured traces?

We do, but if we only rely on structured recordings then we don't need the span to be verbose. Only verbose spans accumulate free form logs, and knz had recommended sticking to lightweight structured protos to avoid perf impacts.

re: whether we should have these kinds of recordings at all, I believe they would serve as visual cues when pulling a jobs trace to help narrow down the "phase" in which a job is stuck?

miretskiy · 2021-05-26T17:39:04Z

I meant types.StringValue -- it's a proto, and it has a "Value" in it. What do we get when using bulk specific proto messages?

adityamaru · 2021-05-26T17:53:54Z

I meant types.StringValue -- it's a proto, and it has a "Value" in it. What do we get when using bulk specific proto messages?

oh yes! That I'm 100% onboard with. I just made it its own proto for the time being incase I figured we needed more bulk-specific fields in it. Will change it to types.StringValue if that is not the case.

pbardea · 2021-05-27T11:37:26Z

pkg/ccl/backupccl/backup.proto

+  message Spans {
+    repeated roachpb.Span spans = 1 [(gogoproto.nullable) = false];
+  }
+  map<int32, Spans> node_to_spans = 1 [(gogoproto.nullable) = false];


One thing I'm curious about is how large we can/want these protos to be? Something like node_to_spans would grow O(ranges). Is this okay?

Yeah crossed my mind while I was adding this proto. I don't have a very good intuition about what serializing/deserializing an O(ranges) proto costs us. I believe we will need to serialize/deserialize if we pull the trace from a node other than the coordinator node, as well as when we use the existing payloads_for_span generator -

cockroach/pkg/sql/sem/builtins/generator_builtins.go

Line 1552 in 723ded9

payload, err := protoreflect.MessageToJSON(item, true /* emitDefaults */)

.

I thought it would be valuable to know which span is being executed on where, but maybe we can get away with adding the span to the trace at the ExportRequest level, when the node actually serves the export request?

adityamaru · 2021-06-03T00:01:59Z

The first 4 commits are from the refactor in #65576, so the last two commits are where the changes have been made. I'm considering adding a separate PR with trace recordings for the external storage implementations.

adityamaru · 2021-06-03T00:50:08Z

pkg/ccl/backupccl/backup.proto

+    string retryable_error = 5;
+  }
+  message Response {
+    util.hlc.Timestamp resp_received_time = 1 [(gogoproto.nullable) = false];


maybe duration would be more useful here?

changed to duration since that seems more useful than a timestamp.

pbardea

The events LGTM; same comments from the other tracing PR also apply to this (re: redaction and possible truncation of the error messages)

pbardea · 2021-06-21T12:39:13Z

pkg/ccl/backupccl/backup.proto

@@ -170,3 +170,15 @@ message RestoreProgress {
  int64 progressIdx = 2;
  roachpb.Span dataSpan = 3 [(gogoproto.nullable) = false];
 }
+
+message BackupPlanningTraceEvent {


nit: perhaps BackupProcessorPlanningTraceEvent?

adityamaru · 2021-06-28T21:59:29Z

The events LGTM; same comments from the other tracing PR also apply to this (re: redaction and possible truncation of the error messages)

Yup, moved the helper to a utils file in tracing, and passed errors through that.

adityamaru · 2021-06-28T22:00:02Z

(not sure if you wanted to have another look at this)

pbardea

LGTM with naming nit

pbardea · 2021-06-29T13:41:03Z

pkg/util/tracing/utils.go

+
+// RedactAndTruncateErrorForTracing redacts the error and truncates the string
+// representation of the error to a fixed length.
+func RedactAndTruncateErrorForTracing(err error) string {


nit: Could we just call this RedactAndTruncateError since it's already in the tracing package (so callers will already see tracing.RedactAndTruncateError?

This change adds trace recordings to noteworthy places during backup exection. Release note: None

adityamaru · 2021-06-29T22:48:47Z

Bazel CI is red because of a macOS cross-build failure. When I trigger a run manually it passes but doesn't seem to reflect here. Going to merge since I don't believe it is related to this PR, will bring it up with dev inf.

bors r=pbardea

craig · 2021-06-29T23:28:09Z

Build succeeded:

GitHub CI (Cockroach)

adityamaru requested review from pbardea, dt and miretskiy May 26, 2021 16:20

pbardea reviewed May 27, 2021

View reviewed changes

adityamaru force-pushed the add-backup-tracing branch from 6759808 to b19b02f Compare June 3, 2021 00:00

adityamaru requested a review from pbardea June 3, 2021 00:02

adityamaru marked this pull request as ready for review June 3, 2021 00:40

adityamaru requested review from a team and removed request for a team June 3, 2021 00:40

adityamaru commented Jun 3, 2021

View reviewed changes

adityamaru force-pushed the add-backup-tracing branch from b19b02f to ac94438 Compare June 4, 2021 15:00

pbardea reviewed Jun 21, 2021

View reviewed changes

adityamaru force-pushed the add-backup-tracing branch 2 times, most recently from aa94417 to 0620495 Compare June 28, 2021 19:10

adityamaru requested a review from pbardea June 28, 2021 21:59

pbardea approved these changes Jun 29, 2021

View reviewed changes

backupccl: add trace recordings to the backup execution

94d9723

This change adds trace recordings to noteworthy places during backup exection. Release note: None

adityamaru force-pushed the add-backup-tracing branch from 0620495 to 94d9723 Compare June 29, 2021 21:38

craig bot merged commit d4627db into cockroachdb:master Jun 29, 2021

adityamaru deleted the add-backup-tracing branch June 29, 2021 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: add some span recordings to the backup resumer #65725

backupccl: add some span recordings to the backup resumer #65725

adityamaru commented May 26, 2021 •

edited

Loading

cockroach-teamcity commented May 26, 2021

adityamaru commented May 26, 2021

miretskiy commented May 26, 2021

adityamaru commented May 26, 2021

miretskiy commented May 26, 2021

adityamaru commented May 26, 2021

pbardea May 27, 2021

adityamaru May 27, 2021

adityamaru commented Jun 3, 2021 •

edited

Loading

adityamaru Jun 3, 2021

adityamaru Jun 4, 2021

pbardea left a comment

pbardea Jun 21, 2021

adityamaru Jun 28, 2021

adityamaru commented Jun 28, 2021

adityamaru commented Jun 28, 2021

pbardea left a comment

pbardea Jun 29, 2021

adityamaru Jun 29, 2021

adityamaru commented Jun 29, 2021

craig bot commented Jun 29, 2021

backupccl: add some span recordings to the backup resumer #65725

backupccl: add some span recordings to the backup resumer #65725

Conversation

adityamaru commented May 26, 2021 • edited Loading

cockroach-teamcity commented May 26, 2021

adityamaru commented May 26, 2021

miretskiy commented May 26, 2021

adityamaru commented May 26, 2021

miretskiy commented May 26, 2021

adityamaru commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru commented Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru commented Jun 28, 2021

adityamaru commented Jun 28, 2021

pbardea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru commented Jun 29, 2021

craig bot commented Jun 29, 2021

adityamaru commented May 26, 2021 •

edited

Loading

adityamaru commented Jun 3, 2021 •

edited

Loading