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

runtime/debug: add SetCrashOutput options #67182

Closed
adonovan opened this issue May 5, 2024 · 21 comments
Closed

runtime/debug: add SetCrashOutput options #67182

adonovan opened this issue May 5, 2024 · 21 comments

Comments

@adonovan
Copy link
Member

adonovan commented May 5, 2024

The debug.SetCrashOutput function was added for go1.23, but has not yet been released. After discussion with @aclements, we agree that support for JSON-formatted crash output containing all the information of runtime.CallersFrames is a desirable feature, since it frees the consumer of the crash from having to be a process of the same executable as the producer of the crash (in order to make sense of the PCs), and from having to think about relative address mappings--and no doubt for other reasons as well.

We propose to change the signature of SetCrashOutput so that it has a second parameter of type CrashOptions, which is defined as an initially empty struct. (We expect to add JSON bool in future, but neither of has time to implement it before the go1.23 freeze.)

package debug // "runtime/debug"

type CrashOptions struct{} // for future expansion

func SetCrashOutput(f *os.File, opts CrashOptions)

This change may break clients that depend on unreleased go1.23 features, such as x/telemetry. As a tactical matter, I suggest we add a variadic parameter ...CrashOptions, update x/telemetry, and then remove the ..., all within a few hours. [Edit: doesn't work; clients will need to use typeswitch during the transition.]

@Stavrospanakakis
Copy link

@adonovan I tried to implement the solution by adding the variadic parameter ...CrashOptions.

However, after running the ./make.bash command, I got the following error:

 ./make.bash
Building Go cmd/dist using /usr/local/go. (go1.22.2 darwin/arm64)
Building Go toolchain1 using /usr/local/go.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
Building Go toolchain3 using go_bootstrap and Go toolchain2.
Building packages and commands for darwin/arm64.
# cmd/vendor/golang.org/x/telemetry/internal/crashmonitor
/Users/user/go/src/cmd/vendor/golang.org/x/telemetry/internal/crashmonitor/crash_go123.go:13:19: cannot use debug.SetCrashOutput (value of type func(f *os.File, opts ...debug.CrashOptions) error) as func(*os.File) error value in assignment
go tool dist: FAILED: /Users/user/go/pkg/tool/darwin_arm64/go_bootstrap install cmd: exit status 1

I suppose that the reason is the following line:

var setCrashOutput func(*os.File) error // = runtime.SetCrashOutput on go1.23+

For this reason, I opened these 2 pull requests #67192 golang/telemetry#8

What do you think?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/583300 mentions this issue: runtime/debug: add the options parameter to SetCrashOutput

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/583278 mentions this issue: internal/crashmonitor: add the options parameter to the setCrashOutput

@adonovan
Copy link
Member Author

adonovan commented May 6, 2024

Good point. Then perhaps we should just preemptively change x/telemetry to typeswitch on SetCrashOutput and do the right thing for both types.

@prattmic
Copy link
Member

prattmic commented May 6, 2024

Marked as release blocker to ensure we make a decision before the release.

@aclements
Copy link
Member

Other uses we discussed for the options struct:

  • Forcing a particular level of GOTRACEBACK for that output
  • Requesting that a user space core dump be in included (much more pie-in-the-sky, obviously)

@findleyr findleyr modified the milestones: Proposal, Go1.23 May 8, 2024
@findleyr
Copy link
Contributor

findleyr commented May 8, 2024

@prattmic I milestoned this for 1.23, rather than Proposal, since I think that affects tooling that searches for 1.23 release blockers. Please correct me if that's wrong.

@matloob please note that this will require re-vendoring x/telemetry. In fact, this will be tricky or impossible to land without breaking one of the builds.

@adonovan
Copy link
Member Author

adonovan commented May 8, 2024

In fact, this will be tricky or impossible to land without breaking one of the builds.

Tricky but not impossible. The plan agreed above is:

  1. change x/telemetry to use the symbol only in call position (thus avoiding a dependency on the precise type of the symbol).
  2. change GOROOT to add a variadic ... parameter (and reject len > 1).
  3. change x/telemetry to provide the options struct.
  4. change GOROOT to remove the "...".

@Stavrospanakakis
Copy link

Stavrospanakakis commented May 8, 2024

@findleyr More specifically, what about the following?

  1. Merging the following PR internal/crashmonitor: update the setCrashOutput definition telemetry#8 that updates the setCrashOutput definition
  • This pull request updates the type of the SetCrashOutput function to prepare the code for step 2
  1. Add a variadic parameter called CrashOptions as done in this PR runtime/debug: add the options parameter to SetCrashOutput #67192
  2. Open another PR to x/telemetry that adds the CrashOptions parameter
  3. Remove the ... from the CrashOptions parameter introduced in step 2

Thoughts?

@findleyr
Copy link
Contributor

findleyr commented May 8, 2024

Aha, thanks.

@adonovan sorry for missing this consideration in the top comment. That plan sounds feasible.

@Stavrospanakakis sure, that seems about right. I'll defer to @adonovan for coordinating these changes if/when the proposal is approved.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to change the recently added

func SetCrashOutput(*os.File)

to be

type CrashOptions struct{}
func SetCrashOutput(*os.File, CrashOptions)

for future extensibility.

We expect that there will be a JSON bool added in a future Go release (perhaps Go 1.24).

@rsc rsc moved this from Incoming to Likely Accept in Proposals May 8, 2024
@ianlancetaylor ianlancetaylor changed the title proposal: runtime/debug.SetCrashOuptut: add Options parameter proposal: runtime/debug.SetCrashOutput: add Options parameter May 15, 2024
@rsc
Copy link
Contributor

rsc commented May 15, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to change the recently added

func SetCrashOutput(*os.File)

to be

type CrashOptions struct{}
func SetCrashOutput(*os.File, CrashOptions)

for future extensibility.

We expect that there will be a JSON bool added in a future Go release (perhaps Go 1.24).

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 15, 2024
@rsc rsc changed the title proposal: runtime/debug.SetCrashOutput: add Options parameter runtime/debug.SetCrashOutput: add Options parameter May 15, 2024
@rsc rsc changed the title runtime/debug.SetCrashOutput: add Options parameter runtime/debug: add SetCrashOutput options May 15, 2024
@rsc rsc changed the title runtime/debug: add SetCrashOutput options proposal: runtime/debug: add SetCrashOutput options May 15, 2024
@rsc rsc changed the title proposal: runtime/debug: add SetCrashOutput options runtime/debug: add SetCrashOutput options May 15, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585378 mentions this issue: internal/crashmonitor: prepare for SetCrashOutput signature change

gopherbot pushed a commit to golang/telemetry that referenced this issue May 15, 2024
The function is about to get a second parameter.
This shim will be removed later this week.

Updates golang/go#67182

Change-Id: I166adda831979b713d429504b57510506bfdff11
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585378
Reviewed-by: Michael Matloob <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585557 mentions this issue: runtime/debug: add SetCrashOutput(...CrashOptions) parameter

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585835 mentions this issue: x/tools: update to x/telemetry@ac8fed8

gopherbot pushed a commit to golang/tools that referenced this issue May 15, 2024
Updates golang/go#67182

Change-Id: I63e115eabb0e6780942add3e60c9a3cb147371be
Reviewed-on: https://go-review.googlesource.com/c/tools/+/585835
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585818 mentions this issue: cmd/vendor/golang.org/x/telemetry: update to ac8fed8

gopherbot pushed a commit that referenced this issue May 15, 2024
This is a placeholder for future options (e.g. JSON).

The parameter is temporarily variadic to avoid breaking
x/telemetry (see CL 585378), but I plan to remove
the "..." later this week.

Updates #67182

Change-Id: I3f6f39455d852f92902f8e3f007d3093cbe555db
Reviewed-on: https://go-review.googlesource.com/c/go/+/585557
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Austin Clements <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585478 mentions this issue: internal/crashmonitor: pass CrashOptions to SetCrashOutput

gopherbot pushed a commit that referenced this issue May 15, 2024
Updates #67182

Change-Id: I14f6a35491e3a58fff2f33285bd13ac706668df6
Reviewed-on: https://go-review.googlesource.com/c/go/+/585818
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Austin Clements <[email protected]>
gopherbot pushed a commit to golang/telemetry that referenced this issue May 15, 2024
Now that CL 585557 has landed in runtime/debug,
we can supply the options.

But we cannot eta-reduce the func literal because
we don't want setCrashOutput's type to mention
debug.CrashOptions, which is a go1.23-only type.

Updates golang/go#67182

Change-Id: I2c3b5cbc4594b6bcd5b5ff43c933d0609cc1c309
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585478
Reviewed-by: Michael Matloob <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585819 mentions this issue: cmd/vendor/golang.org/x/telemetry: update to 9ff3ad9

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585420 mentions this issue: runtime/debug: eliminate temporary variadicity from SetCrashOutput

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585821 mentions this issue: x/tools: update to x/telemetry@9ff3ad9

gopherbot pushed a commit to golang/tools that referenced this issue May 15, 2024
Updates golang/go#67182

Change-Id: If9a225a0058c4c837b90238f993ac0d68783ca3a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/585821
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>
gopherbot pushed a commit that referenced this issue May 16, 2024
Updates #67182

Change-Id: I76b312ccbd1ea98eb2f4e3beec9e8b42e633ea5b
Reviewed-on: https://go-review.googlesource.com/c/go/+/585819
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Austin Clements <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
gopherbot pushed a commit that referenced this issue May 16, 2024
Updates #67182

Change-Id: I33fc8c515f4a9d120262ba30f61aea80ede5e9f8
Reviewed-on: https://go-review.googlesource.com/c/go/+/585420
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Austin Clements <[email protected]>
@adonovan adonovan self-assigned this May 16, 2024
@adonovan
Copy link
Member Author

This is now complete. Please be sure to use tip GOROOT when building tip x/tools or x/telemetry.

Pinkle-pash added a commit to Pinkle-pash/telemetry that referenced this issue Dec 4, 2024
The function is about to get a second parameter.
This shim will be removed later this week.

Updates golang/go#67182

Change-Id: I166adda831979b713d429504b57510506bfdff11
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585378
Reviewed-by: Michael Matloob <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Pinkle-pash added a commit to Pinkle-pash/telemetry that referenced this issue Dec 4, 2024
Now that CL 585557 has landed in runtime/debug,
we can supply the options.

But we cannot eta-reduce the func literal because
we don't want setCrashOutput's type to mention
debug.CrashOptions, which is a go1.23-only type.

Updates golang/go#67182

Change-Id: I2c3b5cbc4594b6bcd5b5ff43c933d0609cc1c309
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/585478
Reviewed-by: Michael Matloob <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

7 participants