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.

Sign up for GitHub

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

Jump to bottom

feature: flightrecorder to enable Go trace #2985

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feature: flightrecorder to enable Go trace #2985

wants to merge 4 commits into from
+309 −13

Conversation

szuecs
Copy link
Member

@szuecs szuecs commented Mar 14, 2024 •
edited
Loading

feature: flightrecorder to enable Go trace

https://go.dev/blog/execution-traces-2024
https://pkg.go.dev/golang.org/x/exp/trace#FlightRecorder

wdyt?

Ideas from discussion

  • enable flightrecorder by filter
  • write to http endpoint PUT (think of cluster local service that collects the trace and make it available or S3)
  • test: separate client and server similar to stdlib to benchmark overhead only in proxy

RomanZavodskikh
RomanZavodskikh reviewed Mar 15, 2024
View reviewed changes
proxy/proxy.go Outdated
return
}
// Write it to a file.
if err := os.WriteFile("trace.out", b.Bytes(), 0o755); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

How are we going to fetch this file if needed? Manually copying it from the pod? Or is there some automation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cite "Ideas from discussion":

write to http endpoint PUT (think of cluster local service that collects the trace and make it available or S3)

RomanZavodskikh
RomanZavodskikh reviewed Mar 15, 2024
View reviewed changes
proxy/proxy.go Outdated Show resolved Hide resolved
AlexanderYastrebov
AlexanderYastrebov reviewed Mar 19, 2024
View reviewed changes
config/config.go Outdated
flag.IntVar(&cfg.FlightRecorderSize, "flight-recorder-size", 0, "max flight-recorder trace data size")
flag.DurationVar(&cfg.FlightRecorderPeriod, "flight-recorder-period", 0, "sets the approximate time duration that the flight recorder's circular buffer represents.")
flag.DurationVar(&cfg.FlightRecorderProxyTookTooLong, "flight-recorder-proxy-took-too-long", 0, "sets the threshold, if proxy took longer than that the flight recorder will write out a trace.")
flag.StringVar(&cfg.FlightRecorderTargetURL, "flight-recorder-target-url", "", "sets the flight recorder target URL that is used to write out the trace to.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better as a filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do you think it's better that people have to pass their own URL into the filter?

@szuecs szuecs force-pushed the feature/flight-recorder branch 2 times, most recently from b23465b to c42035a Compare March 20, 2024 18:56
@szuecs
Copy link
Member Author

szuecs commented Mar 20, 2024 •
edited
Loading

I get only errors for Go 1.22 support:

+ compile_go_fuzzer github.com/zalando/skipper/fuzz/fuzz_targets FuzzParseRouteGroupsJSON FuzzParseRouteGroupsJSON gofuzz
/usr/local/bin/compile_go_fuzzer: line 28: cd: /root/go/src/github.com/zalando/skipper/fuzz/fuzz_targets: No such file or directory
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
/usr/local/bin/compile_go_fuzzer: line 32: cd: github.com/zalando/skipper@*: No such file or directory
/src/skipper
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
Running go-fuzz -tags gofuzz -func FuzzParseRouteGroupsJSON -o FuzzParseRouteGroupsJSON.a github.com/zalando/skipper/fuzz/fuzz_targets
2024/03/20 18:48:40 failed to load packages:go [-e -json -compiled=false -test=false -export=false -deps=false -find=true -buildmode c-archive -gcflags all=-d=libfuzzer -tags gofuzz,gofuzz_libfuzzer,libfuzzer,gofuzz -trimpath -gcflags syscall=-d=libfuzzer=0 -- github.com/zalando/skipper/fuzz/fuzz_targets]: exit status 1: go: downloading go1.22 (linux/amd64)
go: download go1.22 for linux/amd64: toolchain not available
2024-03-20 18:48:41,038 - root - ERROR - Building fuzzers failed.
2024-03-20 18:48:41,038 - root - ERROR - Error building fuzzers for (commit: 48f73d71fea2fbd879fcec320fae5e35443243d9, pr_ref: refs/pull/2985/merge).

Without forcing Go1.22 I get:

Running go-fuzz -tags gofuzz -func FuzzParseRouteGroupsJSON -o FuzzParseRouteGroupsJSON.a github.com/zalando/skipper/fuzz/fuzz_targets
# github.com/zalando/skipper/proxy
proxy/proxy.go:322:24: undefined: trace.FlightRecorder
proxy/proxy.go:424:40: undefined: trace.FlightRecorder
2024/03/20 18:58:30 failed to build packages:exit status 1
2024-03-20 18:58:33,169 - root - ERROR - Building fuzzers failed.
2024-03-20 18:58:33,169 - root - ERROR - Error building fuzzers for (commit: 368aca29104401b8a16a6e8c791e06026d22b1fb, pr_ref: refs/pull/2985/merge).

I don't really know why: https://pkg.go.dev/golang.org/x/exp/trace#FlightRecorder should not depend on Go version, because x/ is a separate package. Maybe it's really an issue created by go-fuzz. Maybe fixed by google/oss-fuzz#11677

@szuecs szuecs force-pushed the feature/flight-recorder branch 4 times, most recently from 0498acc to baa60cf Compare July 1, 2024 13:54
@szuecs szuecs marked this pull request as ready for review July 1, 2024 15:03
@szuecs szuecs added the minor no risk changes, for example new filters label Jul 1, 2024
@szuecs szuecs force-pushed the feature/flight-recorder branch 2 times, most recently from b073f6c to fadd0fe Compare September 6, 2024 11:00
szuecs
szuecs commented Sep 10, 2024
View reviewed changes
proxy/proxy.go Outdated

switch p.flightRecorderURL.Scheme {
case "file":
if err := os.WriteFile(p.flightRecorderURL.Path, b.Bytes(), 0o644); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

  • this overrides the file if we have more than one happening
  • file rotation

szuecs
szuecs commented Sep 10, 2024
View reviewed changes
proxy/proxy.go Outdated
} else {
p.log.Infof("FlightRecorder wrote %d bytes to trace file %q", b.Len(), p.flightRecorderURL.Path)
}
case "http", "https":
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe SupportListener expose /trace/dump

@szuecs szuecs force-pushed the feature/flight-recorder branch 2 times, most recently from 98f84bc to 904c64c Compare September 10, 2024 15:20
szuecs
szuecs commented Sep 10, 2024
View reviewed changes
proxy/proxy.go
p.FlightRecorder.Stop()
p.FlightRecorder = nil
} else {
go func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

forever runs and tries to be an efficient way of handling multiple goroutines fire "we are slow" and write only once in an hour trace data to check.

@szuecs szuecs force-pushed the feature/flight-recorder branch 3 times, most recently from 1585974 to 22c3e06 Compare September 10, 2024 21:08
@szuecs
feature: flightrecorder to enable Go trace
40a3067 
feature: allow configuration for Go x/trace.FlightRecorder
pass default period to define what means skipper to be slow

Signed-off-by: Sandor Szücs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomanZavodskikh RomanZavodskikh RomanZavodskikh left review comments

@AlexanderYastrebov AlexanderYastrebov AlexanderYastrebov left review comments

Assignees
No one assigned
Labels
minor no risk changes, for example new filters
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@szuecs @AlexanderYastrebov @RomanZavodskikh