-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
bench/rttanalysis: stop printing text trace, write jaeger #73790
bench/rttanalysis: stop printing text trace, write jaeger #73790
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/bench/rttanalysis/rtt_analysis_bench.go, line 156 at r1 (raw file):
jaegerJson, err := r.ToJaegerJSON(tc.Stmt, "", "n0") require.NoError(b, err) path := filepath.Join(dir, strings.Replace(b.Name(), "/", "_", -1)) + ".jaeger.json"
Is there a way to get this file from CI if it failed? Maybe that's not important because we always try to repro the failure locally anyway :)
This commit changes the behavior on failure of the rttanalysis tests. Before it would print a textual trace. That was pretty much useless and it was very loud. Now it'll write the jaeger trace json to a file in the logging directory. Hopefully that'll be more useful to inspect the difference between the expectation and the observed result. Release note: None
bf2e53f
to
6324c69
Compare
73791: bench/rttanalysis: make test more lenient r=ajwerner a=ajwerner I don't know why this failed. I made a separate PR (#73790) to help debug it later. For now, just allow more leniency. I'm contemplating allowing the RTTs to always have one extra on the high side. I'd like to understand these better, but not today. Fixes #73771. Release note: None 73804: ci: fix `teamcity-trigger` when running under bazel r=rail a=rickystewart This was just a typo -- the two branches here are supposed to have different logic. This should fix an error we see in CI where stress builds fail with: While parsing option --test_timeout=40m0s: '40m0s' is not an int Release note: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan)
pkg/bench/rttanalysis/rtt_analysis_bench.go, line 156 at r1 (raw file):
Previously, chengxiong-ruan (Chengxiong Ruan) wrote…
Is there a way to get this file from CI if it failed? Maybe that's not important because we always try to repro the failure locally anyway :)
Yes, this goes out of its way to use the log dir because we collect the log dir from failing tests.
Build succeeded: |
This commit changes the behavior on failure of the rttanalysis tests.
Before it would print a textual trace. That was pretty much useless and
it was very loud. Now it'll write the jaeger trace json to a file in the
logging directory. Hopefully that'll be more useful to inspect the difference
between the expectation and the observed result.
Release note: None