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

workload/schemachange: export OTLP traces to disk #114770

Merged
merged 1 commit into from Nov 30, 2023

Conversation

chrisseto
Copy link
Contributor

@chrisseto chrisseto commented Nov 20, 2023

aa05213 workload/schemachange: export OTLP traces to disk

Previous commits instrumented the schemachange workload with OTeL but
did not provide a way to access the output spans.

This commit implements a OTLP client that exports to a gzipped JSON+nl
file. As noted in the client's documentation, tooling for this format is
sparse but JSON wrangling tools can provide extremely rich insights. Due
to versioning issues, loading traces into a tool like Jaeger will
require a small amount of post-processing.

Epic: none
Release note (cli change): workload schemachange now writes a
.otlp.ndjson.gz archive containing OTLP trace bundles for debugging
purposes.

@chrisseto chrisseto requested review from a team as code owners November 20, 2023 22:47
@chrisseto chrisseto requested review from DarrylWong and renatolabs and removed request for a team November 20, 2023 22:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm! just had minor questions

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @DarrylWong, and @renatolabs)


pkg/workload/schemachange/schemachange.go line 119 at r2 (raw file):

			`If provided, transactions will be written to this file in JSON form`)
		s.flags.StringVar(&s.traceFilePath, `trace-file`, "",
			`The file to write OTeL traces to. Defaults to schemachange-workload.{timestamp}.otlp.jsonnl.gz`)

nit: should we juse use *.json.gz or *.ndjson.gz? (same for other usages below)


pkg/workload/schemachange/tracing.go line 119 at r2 (raw file):

	var err error
	c.file, err = os.OpenFile(c.Path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0660)

does this mean we'll append to the file if it exists already? is it more usable to recreate the file?

@chrisseto chrisseto force-pushed the rsw-otlp-export branch 3 times, most recently from 8eb9662 to caff432 Compare November 29, 2023 19:57
Copy link
Contributor Author

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @rafiss, and @renatolabs)


pkg/workload/schemachange/schemachange.go line 119 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should we juse use *.json.gz or *.ndjson.gz? (same for other usages below)

Fixed!


pkg/workload/schemachange/tracing.go line 119 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

does this mean we'll append to the file if it exists already? is it more usable to recreate the file?

Yes it does and now that you ask not in the slightest. If the file was just ndjson, we could append just fine but the addition of gzip means we'll just corrupt the archive 😬

Fixed to just use os.Create. Good catch!

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm! just asked about a pending todo

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @DarrylWong, and @renatolabs)


pkg/workload/schemachange/schemachange.go line 822 at r3 (raw file):

	path := s.traceFilePath
	if path == "" {
		// TODO(chrisseto): Before merge ensure this default output is to a

were you able to check this?

Previous commits instrumented the schemachange workload with OTeL but
did not provide a way to access the output spans.

This commit implements a OTLP client that exports to a gzipped JSON+nl
file. As noted in the client's documentation, tooling for this format is
sparse but JSON wrangling tools can provide extremely rich insights. Due
to versioning issues, loading traces into a tool like Jaeger will
require a small amount of post-processing.

Epic: none
Release note (cli change): workload schemachange now writes a
.otlp.ndjson.gz archive containing OTLP trace bundles for debugging
purposes.
@chrisseto chrisseto force-pushed the rsw-otlp-export branch 2 times, most recently from e974ae0 to aa05213 Compare November 30, 2023 17:00
Copy link
Contributor Author

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @rafiss, and @renatolabs)


pkg/workload/schemachange/schemachange.go line 822 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

were you able to check this?

Yep! It works as expected though it's not super ergonomic to access as I noted in slack. I might spend some later trying to make the logged directory a clickable URL in TC.

@chrisseto
Copy link
Contributor Author

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Nov 30, 2023

Build succeeded:

@craig craig bot merged commit 0d9214c into cockroachdb:master Nov 30, 2023
9 checks passed
@chrisseto chrisseto deleted the rsw-otlp-export branch November 30, 2023 22:20
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

3 participants