Skip to content

feat: summarize information from debug logs#632

Open
CatalinSnyk wants to merge 1 commit into
feat/snyk-doctorfrom
feat/CLI-1573_extract_events_from_log
Open

feat: summarize information from debug logs#632
CatalinSnyk wants to merge 1 commit into
feat/snyk-doctorfrom
feat/CLI-1573_extract_events_from_log

Conversation

@CatalinSnyk

@CatalinSnyk CatalinSnyk commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Description

  • Adding some basic matching for notable/useful information from debug logs.
image

Checklist

  • Tests added and all succeed (make test)
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint)
  • Test your changes work for the CLI - PR

@snyk-io

snyk-io Bot commented Jun 19, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io

snyk-io Bot commented Jun 19, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@CatalinSnyk CatalinSnyk marked this pull request as ready for review June 19, 2026 13:35
@CatalinSnyk CatalinSnyk requested review from a team as code owners June 19, 2026 13:35
@snyk-pr-review-bot

This comment has been minimized.

@CatalinSnyk CatalinSnyk force-pushed the feat/CLI-1573_extract_events_from_log branch from 80bc9f4 to 01b2ac8 Compare June 19, 2026 13:44
@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Toolchain Incompatibility 🟠 [major]

The function writeSection uses strings.SplitSeq, which was introduced in Go 1.23. If the project's go.mod or the environment's toolchain is at a lower version (e.g., Go 1.21 or 1.22, common in many enterprise CI environments), this will cause a compilation failure. Use strings.Split or bufio.Scanner for better compatibility.

for line := range strings.SplitSeq(content, "\n") {
High Memory Pressure 🟡 [minor]

In Summarize, the code creates a new slice of strings msgs with the same length as the lines slice, effectively doubling the memory required to hold the log content. For very large debug logs (which can exceed 100MB in complex CLI operations), this may lead to unnecessary OOMs. Consider processing the log line-by-line using a scanner or modifying the strings in place if possible.

msgs := make([]string, len(lines))
for i, line := range lines {
📚 Repository Context Analyzed

This review considered 12 relevant code sections from 11 files (average relevance: 1.00)

Comment on lines +19 to +21
for _, h := range summary.Highlights {
fmt.Fprintf(&sb, " L%d [%s] %s\n", h.Line, h.Kind, h.Message)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there any scenario where Line, Kind or Message would be empty / not present?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think not, Line is always i+1 ≥ 1; Kind is always one of the four constants set by classify; and classify("") returns false, so empty/whitespace messages never become highlights.

Comment on lines +162 to +164
func isTableRow(msg string) bool {
return msg != "" && (msg[0] == ' ' || msg[0] == '\t' || tableRowRe.MatchString(msg))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems very hacky, not sure if there are better ways of doing this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll follow-up on it

Comment on lines +109 to +125
func headerSpan(msgs []string) span {
start := -1
for i, msg := range msgs {
if strings.HasPrefix(msg, "Version:") {
start = i
break
}
}
if start < 0 {
return span{-1, -1}
}
end := start
for i := start + 1; i < len(msgs) && isTableRow(msgs[i]); i++ {
end = i
}
return span{start, end}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm thinking if is possible to work with templates instead of doing these manipulations... Have you considered those / had impediments regarding them?

@CatalinSnyk CatalinSnyk Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not quite sure, what kind of templates did you have in mind?

Templates that we have in GAF (for our presenter) are better for rendering: if we used templates for rendering the CLI header/footer then I think it could be a bit more viable, but for matching the current log header/footer section I don't think it would make much sense.

@danskmt

danskmt commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

@CatalinSnyk also, bot left some comments that might be relevant:
image

@CatalinSnyk

Copy link
Copy Markdown
Contributor Author

@CatalinSnyk also, bot left some comments that might be relevant:

  • The CLI is on 1.26 (same as the version in go.mod in this project) - I don't think this should be an issue
  • This doesn't seem quite right, since we are matching we would return substrings. The scanner/streaming of logs could be something we follow-up on ticket, I'm more keen to keep it simple for now.

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.

2 participants