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

stmtdiagnostics: break into chunks #50974

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

RaduBerinde
Copy link
Member

Implement breaking support bundles into chunks. The bundles contain
traces which can be large, resulting in "command is too large" errors.

Release note (bug fix): better support for large statement diagnostic
bundles.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member

I guess this will fix #50960?

@andreimatei
Copy link
Contributor

I've ran into some really big traces too, because of a him reader. I was thinking we put limits on the number of children a span can record. Like we're only keep track of the first and the last

@RaduBerinde
Copy link
Member Author

Hm yeah I think it would fix that issue. I didn't know we collect bundles in tpch tests

@yuzefovich
Copy link
Member

yuzefovich commented Jul 5, 2020

I didn't know we collect bundles in tpch tests

We don't. There is an occasional performance slowness that we're trying to track down, so we temporarily collect a bundle after that slowness occurs.

Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @RaduBerinde)


pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 41 at r1 (raw file):

	10*time.Second)

var bundleChunkSize = 128 * 1024

I would make them a lot bigger - say, 10MB. I think it'd be good to keep the majority of bundles in one chunk.


pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 41 at r1 (raw file):

	10*time.Second)

var bundleChunkSize = 128 * 1024

let's not make this a global; we always run into trouble with globals sooner or later. Let's either plumb it through to the Registry ctor from the TestServerArgs -> server.Config.SQLConfig, or make it a cluster setting.

Copy link
Member Author

@RaduBerinde RaduBerinde 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 @andreimatei)


pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 41 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I would make them a lot bigger - say, 10MB. I think it'd be good to keep the majority of bundles in one chunk.

10MB is in the same ballpark as the 60MB limit we've been hitting.. I made it 1MB (I think that's what we recommend to users).


pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 41 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

let's not make this a global; we always run into trouble with globals sooner or later. Let's either plumb it through to the Registry ctor from the TestServerArgs -> server.Config.SQLConfig, or make it a cluster setting.

Done.

Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @RaduBerinde)


pkg/sql/stmtdiagnostics/statement_diagnostics.go, line 41 at r1 (raw file):

Previously, RaduBerinde wrote…

10MB is in the same ballpark as the 60MB limit we've been hitting.. I made it 1MB (I think that's what we recommend to users).

I think you were running into kv.raft.command.max_size which defaults to 64MB. Anything less should work.
For user keys, there's another consideration that applies - which is that we can't split ranges between versions of the same key - so if you overwrite a large key a lot you can get in trouble. But here we never overwrite. That's why I was thinking we can go pretty high.
I'm good with 1MB too though.

@RaduBerinde
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 6, 2020

Build failed

Implement breaking support bundles into chunks. The bundles contain
traces which can be large, resulting in "command is too large" errors.

Release note (bug fix): better support for large statement diagnostic
bundles.
@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 6, 2020

Build succeeded

@craig craig bot merged commit 6b7b297 into cockroachdb:master Jul 6, 2020
@RaduBerinde RaduBerinde deleted the stmt-diag-chunks branch July 15, 2020 18:32
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.

4 participants