search: add support for streaming search #485

stefanhengl · 2021-03-04T12:27:44Z

Stacked on top of #484

This adds the flag -stream to the search sub-command. Searches
submitted with -stream are routed to the endpoint search/stream
instead of the GraphQL api.

In addition, users can set -display <int> to set the display limit.

We will add support for JSON in a follow-up PR.

mrnugget · 2021-03-04T13:10:18Z

I'll take a deeper look, but please add a changelog for these additions :)

mrnugget

Approved but left a few comments, because I think the code can be made cleaner and easier to understand/maintain.

Also: no tests? 😬

mrnugget · 2021-03-04T13:20:21Z

internal/api/flags.go

+	return &Flags{
+		trace:              flagSet.Bool("trace", false, "Log the trace ID for requests. See https://docs.sourcegraph.com/admin/observability/tracing"),
+		insecureSkipVerify: flagSet.Bool("insecure-skip-verify", false, "Skip validation of TLS certificates against trusted chains"),
+	}


Why the duplication here? The NewFlags already returns a *Flags with both of these fields.

Edit: And wouldn't parsing fail if someone sets the get-curl flag, for example?

Edit: And wouldn't parsing fail if someone sets the get-curl flag, for example?

parsing only fails if someone calls search with -stream AND -get-curl, which is intended. -curl and -dump make a lot less sense for streaming than for the GraphQL API. I assume the use case for get-curl is to pipe the output to something like jq? The streaming endpoint does not return valid JSON, but a stream of events that contain JSON.

we could make get curl work though? My original streaming client was curl :D But yeah, until we have stabalized the API properly I don't want people to start writing there own clients.

mrnugget · 2021-03-04T13:22:17Z

cmd/src/search.go

+			}
+			return streamHandler(argsWOStream)
+		}
+


If you define -stream as a noop on the other flagset that you then use in streamHandler you wouldn't have to do this here, right?

yes, but then we mess with the auto-documentation. Suddenly the "streaming search" has a "-stream" flag which is odd. I traded odd code against odd documentation.

Why not create a separate stream-search command?

It is based on a discussion we had around exhaustive search which is documented in RFC 288. At some point we want phase out -stream and replace search with streaming search. That is part of the reason why I wrote it as handler.

This is kind of weird though:

$ ./src search -stream -get-curl foo flag provided but not defined: -get-curl Usage of streaming search: -display int Limit the number of results that are displayed. Note that the statistics continue to report all results. (default -1) -insecure-skip-verify Skip validation of TLS certificates against trusted chains -trace Log the trace ID for requests. See https://docs.sourcegraph.com/admin/observability/tracing

Given we intend on replacing search with stream search in the future, can't we make the stream search stuff not use its own flagset. I would think an approach like

streaming.Search(streaming.Opts{ Query: flagSet.Args(), Trace: *trace, Display: *display, InsecureSkipVerify: *insecureSkipVerify, })

would make sense. Or even you pass in the http client you use that hides the trace/insecure stuff.

That way you can also move the large amounts of business logic out of ./cmd/src into ./internal/streaming

I actually like this behavior, but since both reviewers commented on this I may be on the wrong track here. I refactored it to be a simple function call instead of a handler and moved parts of the logic to internal/streaming.

mrnugget · 2021-03-04T13:25:23Z

cmd/src/search_stream.go

+		},
+		OnMatches: func(matches []streaming.EventMatch) {
+			for _, match := range matches {
+				if file, ok := match.(*streaming.EventFileMatch); ok {


How about a switch match := match.(type) instead of if and continue?

mrnugget · 2021-03-04T13:27:25Z

cmd/src/search_stream.go

+					},
+					)
+					if err != nil {
+						fmt.Printf("ExecuteTemplate: %s\n", err)


Why not print all of these error messages stderr? (You can use flagSet.Output() as below)

mrnugget · 2021-03-04T13:31:35Z

cmd/src/search_stream.go

+	defer resp.Body.Close()
+
+	// Process response.
+	err = streaming.Decoder{


I think I'd personally pull some of this out to make it more readable. All of these callbacks require only the template (and stderr, see below), so you could use closures to make it flatter:

func newDecoder(t *template.Template) streaming.Decoder { return streaming.Decoder{ OnProgress: onProgress(t), // [...] } } func onProgress(t *template.Template) func(*streaming.Progress) { return func(progress *streaming.Progress) { // We only show the final progress. if !progress.Done { return } err = t.ExecuteTemplate(os.Stdout, "progress", progress) if err != nil { fmt.Printf("ExecuteTemplate: %s\n", err) } return } }

mrnugget · 2021-03-04T13:33:51Z

cmd/src/search_stream.go

+					err = t.ExecuteTemplate(os.Stdout, "commit", struct {
+						SourcegraphEndpoint string
+						*streaming.EventCommitMatch
+					}{
+						SourcegraphEndpoint: cfg.Endpoint,
+						EventCommitMatch:    commit,
+					})


I think you could clean up this render code a bit if you introduce something like a Context struct that has all of the fields defined but on which you only set the current struct.

Yeah, I also went back and forth about this one. I think the current approach is pretty straightforward although maybe a bit verbose. If we use a common context struct then the templates get more difficult to read. I will see if I can find something better.

stefanhengl · 2021-03-04T15:45:37Z

Also: no tests? 😬

The PRs are stacked. You reviewed before I had the chance to push the 3rd PR with tests :-).

EDIT: Acutally ended up closing the test-PR because (#486). I rewrote the tests and pushed here.

stefanhengl · 2021-03-05T15:39:23Z

@keegancsmith PTAL. Tests are now integrated in this PR.

keegancsmith · 2021-03-09T07:06:18Z

cmd/src/search.go

+			}
+			return streamHandler(argsWOStream)
+		}
+


This is kind of weird though:

$ ./src search -stream -get-curl foo flag provided but not defined: -get-curl Usage of streaming search: -display int Limit the number of results that are displayed. Note that the statistics continue to report all results. (default -1) -insecure-skip-verify Skip validation of TLS certificates against trusted chains -trace Log the trace ID for requests. See https://docs.sourcegraph.com/admin/observability/tracing

Given we intend on replacing search with stream search in the future, can't we make the stream search stuff not use its own flagset. I would think an approach like

streaming.Search(streaming.Opts{ Query: flagSet.Args(), Trace: *trace, Display: *display, InsecureSkipVerify: *insecureSkipVerify, })

would make sense. Or even you pass in the http client you use that hides the trace/insecure stuff.

That way you can also move the large amounts of business logic out of ./cmd/src into ./internal/streaming

This adds the flag `-stream` to the `search` sub-command. Searches submitted with `-stream` are routed to the endpoint `search/stream` instead of the GraphQL api. In addition, users can set `-display <int>` to set the display limit. We will add support for JSON in a follow-up PR.

This removes the dedicated flagSet for streaming search and move parts of the logic to internal streaming.

This adds the flag `-stream` to the `search` sub-command. Searches submitted with `-stream` are routed to the endpoint `search/stream` instead of the GraphQL API. In addition, users can set `-display <int>` to set the display limit. We will add support for JSON in a follow-up PR.

stefanhengl requested a review from keegancsmith March 4, 2021 12:27

stefanhengl force-pushed the sh/support-streaming branch 2 times, most recently from 5c13ca5 to c881c3e Compare March 4, 2021 12:40

mrnugget approved these changes Mar 4, 2021

View reviewed changes

stefanhengl mentioned this pull request Mar 4, 2021

search: add tests for search_streaming.go #486

Closed

keegancsmith approved these changes Mar 9, 2021

View reviewed changes

Base automatically changed from sh/add-stream-client to main March 9, 2021 09:15

stefanhengl added 6 commits March 9, 2021 10:23

use flagSet.Outpout().Write instead of fmt.Printf

07b7051

use switch instead of if-continue

2020d66

add tests

cc73500

count diff/commit results just like on Cloud

fcf959b

address PR comments

5c951db

This removes the dedicated flagSet for streaming search and move parts of the logic to internal streaming.

stefanhengl force-pushed the sh/support-streaming branch from a96cc4b to 5c951db Compare March 9, 2021 09:27

minor: add newline before trace output

b6017ee

stefanhengl merged commit 35339a2 into main Mar 9, 2021

stefanhengl deleted the sh/support-streaming branch March 9, 2021 09:48

search: add support for streaming search #485

search: add support for streaming search #485

Uh oh!

Conversation

stefanhengl commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrnugget commented Mar 4, 2021

Uh oh!

mrnugget left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanhengl Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanhengl commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanhengl commented Mar 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stefanhengl commented Mar 4, 2021 •

edited

Loading

stefanhengl Mar 9, 2021 •

edited

Loading

stefanhengl commented Mar 4, 2021 •

edited

Loading