Skip to content

Commit 61ce95d

Browse files
SarahFrenchSarahFrench
committed
discovery: benchmarking different approaches
1 parent 02cb91f commit 61ce95d

2 files changed

Lines changed: 255 additions & 3 deletions

File tree

‎internal/command/format/diagnostic.go‎

Lines changed: 195 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package format
55

66
import (
7+
"bufio"
78
"bytes"
89
"fmt"
910
"iter"
@@ -34,6 +35,7 @@ func Diagnostic(diag tfdiags.Diagnostic, sources map[string][]byte, color *color
3435
return DiagnosticFromJSON(viewsjson.NewDiagnostic(diag, sources), color, width)
3536
}
3637

38+
// Edited version from https://github.com/hashicorp/terraform/pull/38710
3739
func DiagnosticFromJSON(diag *viewsjson.Diagnostic, color *colorstring.Colorize, width int) string {
3840
if diag == nil {
3941
// No good reason to pass a nil diagnostic in here...
@@ -133,6 +135,199 @@ func DiagnosticFromJSON(diag *viewsjson.Diagnostic, color *colorstring.Colorize,
133135
return ruleBuf.String()
134136
}
135137

138+
// Original function before the PR
139+
func diagnosticFromJSONOriginal(diag *viewsjson.Diagnostic, color *colorstring.Colorize, width int) string {
140+
if diag == nil {
141+
// No good reason to pass a nil diagnostic in here...
142+
return ""
143+
}
144+
145+
var buf bytes.Buffer
146+
147+
// these leftRule* variables are markers for the beginning of the lines
148+
// containing the diagnostic that are intended to help sighted users
149+
// better understand the information hierarchy when diagnostics appear
150+
// alongside other information or alongside other diagnostics.
151+
//
152+
// Without this, it seems (based on folks sharing incomplete messages when
153+
// asking questions, or including extra content that's not part of the
154+
// diagnostic) that some readers have trouble easily identifying which
155+
// text belongs to the diagnostic and which does not.
156+
var leftRuleLine, leftRuleStart, leftRuleEnd string
157+
var leftRuleWidth int // in visual character cells
158+
159+
switch diag.Severity {
160+
case viewsjson.DiagnosticSeverityError:
161+
buf.WriteString(color.Color("[bold][red]Error: [reset]"))
162+
leftRuleLine = color.Color("[red]│[reset] ")
163+
leftRuleStart = color.Color("[red]╷[reset]")
164+
leftRuleEnd = color.Color("[red]╵[reset]")
165+
leftRuleWidth = 2
166+
case viewsjson.DiagnosticSeverityWarning:
167+
buf.WriteString(color.Color("[bold][yellow]Warning: [reset]"))
168+
leftRuleLine = color.Color("[yellow]│[reset] ")
169+
leftRuleStart = color.Color("[yellow]╷[reset]")
170+
leftRuleEnd = color.Color("[yellow]╵[reset]")
171+
leftRuleWidth = 2
172+
default:
173+
// Clear out any coloring that might be applied by Terraform's UI helper,
174+
// so our result is not context-sensitive.
175+
buf.WriteString(color.Color("\n[reset]"))
176+
}
177+
178+
// We don't wrap the summary, since we expect it to be terse, and since
179+
// this is where we put the text of a native Go error it may not always
180+
// be pure text that lends itself well to word-wrapping.
181+
fmt.Fprintf(&buf, color.Color("[bold]%s[reset]\n\n"), diag.Summary)
182+
183+
f := &snippetFormatter{&buf, diag, color}
184+
f.write()
185+
186+
if diag.Detail != "" {
187+
paraWidth := width - leftRuleWidth - 1 // leave room for the left rule
188+
if paraWidth > 0 {
189+
lines := strings.Split(diag.Detail, "\n")
190+
for _, line := range lines {
191+
if !strings.HasPrefix(line, " ") {
192+
line = wordwrap.WrapString(color.Color(line), uint(paraWidth))
193+
}
194+
fmt.Fprintf(&buf, "%s\n", line)
195+
}
196+
} else {
197+
fmt.Fprintf(&buf, "%s\n", diag.Detail)
198+
}
199+
}
200+
201+
// Before we return, we'll finally add the left rule prefixes to each
202+
// line so that the overall message is visually delimited from what's
203+
// around it. We'll do that by scanning over what we already generated
204+
// and adding the prefix for each line.
205+
var ruleBuf strings.Builder
206+
sc := bufio.NewScanner(&buf)
207+
ruleBuf.WriteString(leftRuleStart)
208+
ruleBuf.WriteByte('\n')
209+
for sc.Scan() {
210+
line := sc.Text()
211+
prefix := leftRuleLine
212+
if line == "" {
213+
// Don't print the space after the line if there would be nothing
214+
// after it anyway.
215+
prefix = strings.TrimSpace(prefix)
216+
}
217+
ruleBuf.WriteString(prefix)
218+
ruleBuf.WriteString(line)
219+
ruleBuf.WriteByte('\n')
220+
}
221+
ruleBuf.WriteString(leftRuleEnd)
222+
ruleBuf.WriteByte('\n')
223+
224+
return ruleBuf.String()
225+
}
226+
227+
// A potential alternative to the changes in https://github.com/hashicorp/terraform/pull/38710
228+
func diagnosticFromJSONBytesIndex(diag *viewsjson.Diagnostic, color *colorstring.Colorize, width int) string {
229+
if diag == nil {
230+
// No good reason to pass a nil diagnostic in here...
231+
return ""
232+
}
233+
234+
var buf bytes.Buffer
235+
236+
// these leftRule* variables are markers for the beginning of the lines
237+
// containing the diagnostic that are intended to help sighted users
238+
// better understand the information hierarchy when diagnostics appear
239+
// alongside other information or alongside other diagnostics.
240+
//
241+
// Without this, it seems (based on folks sharing incomplete messages when
242+
// asking questions, or including extra content that's not part of the
243+
// diagnostic) that some readers have trouble easily identifying which
244+
// text belongs to the diagnostic and which does not.
245+
var leftRuleLine, leftRuleStart, leftRuleEnd string
246+
var leftRuleWidth int // in visual character cells
247+
248+
switch diag.Severity {
249+
case viewsjson.DiagnosticSeverityError:
250+
buf.WriteString(color.Color("[bold][red]Error: [reset]"))
251+
leftRuleLine = color.Color("[red]│[reset] ")
252+
leftRuleStart = color.Color("[red]╷[reset]")
253+
leftRuleEnd = color.Color("[red]╵[reset]")
254+
leftRuleWidth = 2
255+
case viewsjson.DiagnosticSeverityWarning:
256+
buf.WriteString(color.Color("[bold][yellow]Warning: [reset]"))
257+
leftRuleLine = color.Color("[yellow]│[reset] ")
258+
leftRuleStart = color.Color("[yellow]╷[reset]")
259+
leftRuleEnd = color.Color("[yellow]╵[reset]")
260+
leftRuleWidth = 2
261+
default:
262+
// Clear out any coloring that might be applied by Terraform's UI helper,
263+
// so our result is not context-sensitive.
264+
buf.WriteString(color.Color("\n[reset]"))
265+
}
266+
267+
// We don't wrap the summary, since we expect it to be terse, and since
268+
// this is where we put the text of a native Go error it may not always
269+
// be pure text that lends itself well to word-wrapping.
270+
fmt.Fprintf(&buf, color.Color("[bold]%s[reset]\n\n"), diag.Summary)
271+
272+
f := &snippetFormatter{&buf, diag, color}
273+
f.write()
274+
275+
if diag.Detail != "" {
276+
paraWidth := width - leftRuleWidth - 1 // leave room for the left rule
277+
if paraWidth > 0 {
278+
lines := strings.Split(diag.Detail, "\n")
279+
for _, line := range lines {
280+
if !strings.HasPrefix(line, " ") {
281+
line = wordwrap.WrapString(color.Color(line), uint(paraWidth))
282+
}
283+
fmt.Fprintf(&buf, "%s\n", line)
284+
}
285+
} else {
286+
fmt.Fprintf(&buf, "%s\n", diag.Detail)
287+
}
288+
}
289+
290+
// Before we return, we'll finally add the left rule prefixes to each
291+
// line so that the overall message is visually delimited from what's
292+
// around it. We'll do that by iterating over what we already generated
293+
// and adding the prefix for each line.
294+
//
295+
// We intentionally don't use bufio.Scanner here because its default
296+
// token size limit silently discards lines longer than 64KiB, which
297+
// caused diagnostics with very long summaries (e.g. cycle errors in
298+
// large configurations, whose summaries are never word-wrapped) to be
299+
// rendered as an empty box with no text at all.
300+
var ruleBuf strings.Builder
301+
data := buf.Bytes()
302+
303+
ruleBuf.WriteString(leftRuleStart)
304+
ruleBuf.WriteByte('\n')
305+
var i, start int
306+
for i = bytes.IndexByte(data, '\n'); i != -1; {
307+
line := data[start : start+i]
308+
prefix := leftRuleLine
309+
if len(line) == 0 {
310+
// Don't print the space after the line if there would be nothing
311+
// after it anyway.
312+
prefix = strings.TrimSpace(prefix)
313+
}
314+
ruleBuf.WriteString(prefix)
315+
ruleBuf.Write(line)
316+
ruleBuf.WriteByte('\n')
317+
318+
// Record where the next iteration should read from
319+
start = start + i + 1
320+
if start >= len(data) {
321+
break
322+
}
323+
i = bytes.IndexByte(data[start:], '\n')
324+
}
325+
ruleBuf.WriteString(leftRuleEnd)
326+
ruleBuf.WriteByte('\n')
327+
328+
return ruleBuf.String()
329+
}
330+
136331
// DiagnosticPlain is an alternative to Diagnostic which minimises the use of
137332
// virtual terminal formatting sequences.
138333
//
@@ -433,7 +628,6 @@ func (f *snippetFormatter) printTestDiagOutput(diag *viewsjson.DiagnosticTestBin
433628
// It visually distinguishes removed and added lines, helping users identify
434629
// discrepancies between an "actual" (lhsStr) and an "expected" (rhsStr) JSON output.
435630
func (f *snippetFormatter) printJSONDiff(lhsStr, rhsStr string) {
436-
437631
buf := f.buf
438632
color := f.color
439633
// No visible difference in the JSON, so we'll just return

‎internal/command/format/diagnostic_test.go‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"bytes"
99
"fmt"
1010
"regexp"
11+
"strconv"
1112
"strings"
1213
"testing"
1314

@@ -26,7 +27,6 @@ import (
2627
)
2728

2829
func TestDiagnostic(t *testing.T) {
29-
3030
tests := map[string]struct {
3131
Diag interface{}
3232
Want string
@@ -461,7 +461,6 @@ func TestDiagnostic(t *testing.T) {
461461
}
462462

463463
func TestDiagnosticPlain(t *testing.T) {
464-
465464
tests := map[string]struct {
466465
Diag interface{}
467466
Want string
@@ -1496,3 +1495,62 @@ func TestDiagnosticFromJSON_longSummary(t *testing.T) {
14961495
t.Errorf("rendered diagnostic does not contain its own summary text\ngot (truncated to 200 chars): %.200s", got)
14971496
}
14981497
}
1498+
1499+
var benchmarkSink string
1500+
1501+
func BenchmarkDiagnosticFromJSON(b *testing.B) {
1502+
cases := []struct {
1503+
name string
1504+
diag *viewsjson.Diagnostic
1505+
}{
1506+
{name: "small", diag: makeDiag(200, 5)},
1507+
{name: "medium", diag: makeDiag(4000, 200)},
1508+
{name: "above64k", diag: makeDiag(bufio.MaxScanTokenSize*2, 4000)}, // Summary size above bufio.MaxScanTokenSize
1509+
}
1510+
1511+
for _, tc := range cases {
1512+
b.Run(tc.name+"/current", func(b *testing.B) {
1513+
b.ReportAllocs()
1514+
colorize := &colorstring.Colorize{}
1515+
b.ResetTimer()
1516+
for b.Loop() {
1517+
benchmarkSink = DiagnosticFromJSON(tc.diag, colorize, 76)
1518+
}
1519+
})
1520+
1521+
b.Run(tc.name+"/bytes_index", func(b *testing.B) {
1522+
b.ReportAllocs()
1523+
colorize := &colorstring.Colorize{}
1524+
b.ResetTimer()
1525+
for b.Loop() {
1526+
benchmarkSink = diagnosticFromJSONBytesIndex(tc.diag, colorize, 76)
1527+
}
1528+
})
1529+
1530+
// During this benchmark the original implementation will show the bug where
1531+
// long inputs are dropped because they're too long for the Scan method.
1532+
b.Run(tc.name+"/original", func(b *testing.B) {
1533+
b.ReportAllocs()
1534+
colorize := &colorstring.Colorize{}
1535+
b.ResetTimer()
1536+
for b.Loop() {
1537+
benchmarkSink = diagnosticFromJSONOriginal(tc.diag, colorize, 76)
1538+
}
1539+
})
1540+
}
1541+
}
1542+
1543+
func makeDiag(summarySize int, detailLines int) *viewsjson.Diagnostic {
1544+
summary := "Cycle: " + strings.Repeat("module.example.node, ", summarySize/24)
1545+
var sb strings.Builder
1546+
for i := 0; i < detailLines; i++ {
1547+
sb.WriteString("detail line ")
1548+
sb.WriteString(strconv.Itoa(i))
1549+
sb.WriteByte('\n')
1550+
}
1551+
return &viewsjson.Diagnostic{
1552+
Severity: viewsjson.DiagnosticSeverityError,
1553+
Summary: summary,
1554+
Detail: sb.String(),
1555+
}
1556+
}

0 commit comments

Comments
 (0)
⚡