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.

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

Add logcli default client test #10100

Closed
wants to merge 2 commits into from

Conversation

trevorwhitney
Copy link
Collaborator

This adds a test for the default client in logcli. Currently it only covers happy path for volume and volume_range. This was per request of @liguozhong resulting from a bug he found. @liguozhong could you add the unhappy path to recreate the bug? I'm still not sure I completely understand what it is. Thanks!

@trevorwhitney trevorwhitney requested a review from a team as a code owner July 28, 2023 22:24
Copy link
Contributor

@liguozhong liguozhong left a comment

Choose a reason for hiding this comment

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

great

codec := queryrange.Codec{}

mux.HandleFunc("/loki/api/v1/index/volume", func(w http.ResponseWriter, request *http.Request) {
volume := queryrange.LokiPromResponse{
Copy link
Contributor

Choose a reason for hiding this comment

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

Very detailed test case, thanks your help. My only doubt here is the return value type of this test.

The return value type here should be logproto.VolumeResponse instead of queryrange.LokiPromResponse, because the code of http.go return logproto.VolumeResponse, and the test construction in http_test.go is also logproto.VolumeResponse

https://github.com/grafana/loki/blob/main/pkg/querier/http.go#L489

        if resp == nil { // Some stores don't implement this
		resp = &logproto.VolumeResponse{Volumes: []logproto.Volume{}}
	}
	if err := queryrange.WriteResponse(r, nil, resp, w); err != nil {
		serverutil.WriteError(err, w)
	}

https://github.com/grafana/loki/blob/main/pkg/querier/http_test.go#L386

	ret := &logproto.VolumeResponse{
		Volumes: []logproto.Volume{
			{Name: `{foo="bar"}`, Volume: 38},
		},
	}

https://github.com/grafana/loki/blob/main/pkg/util/marshal/marshal.go#L132

func WriteVolumeResponseJSON(r *logproto.VolumeResponse, w io.Writer) error {
	s := jsoniter.ConfigFastest.BorrowStream(w)
	defer jsoniter.ConfigFastest.ReturnStream(s)
	s.WriteVal(r)
	s.WriteRaw("\n")
	return s.Flush()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are all inside the querier package, or in the last case, used for querier -> query frontend communication. we only use logproto.Volume for internal communication between components. The HTTP API, and response returned by the query frontend, converts the multiple logproto.Volume responses it gets from the many queriers that handle the split up query and turns it into a queryrange.LokiPromRepsonse. This is because the primary consumer of these endpoints wants the data to look like a metrics response to more easily plot it.

@liguozhong
Copy link
Contributor

This adds a test for the default client in logcli. Currently it only covers happy path for volume and volume_range. This was per request of @liguozhong resulting from a bug he found. @liguozhong could you add the unhappy path to recreate the bug? I'm still not sure I completely understand what it is. Thanks!

My test refers to the result of this test, and the test here also gets this json .{"volumes":...}

https://github.com/grafana/loki/blob/main/pkg/querier/http_test.go#L419C5-L422C41

                                        require.Equal(t,
					`{"volumes":[{"name":"{foo=\"bar\"}","volume":38}]}`,
					strings.TrimSpace(w.Body.String()),

@liguozhong
Copy link
Contributor

liguozhong commented Jul 29, 2023

recreate the bug

logproto.VolumeResponse type is returned in my test, so this bug should appear. because c.doRequest(..) would get an json {"volumes":...} , and DefaultClient.go
https://github.com/grafana/loki/blob/main/pkg/logcli/client/client.go#L197

  var resp loghttp.QueryResponse
 if err := c.doRequest(path, params.Encode(), quiet, &resp); err != nil {
 	return nil, err
 }

can only decode {status:...,data:...} response(loghttp.QueryResponse).

https://github.com/grafana/loki/pull/7324/files#diff-d1ce4d005c2f1eb8b6916c6727c1a955aae21d99e43e3c5ec58203e5c9c423c7R186

mux.HandleFunc("/loki/api/v1/index/series_volume", func(w http.ResponseWriter, request *http.Request) {
		seriesVolume := logproto.VolumeResponse{
			Volumes: []logproto.Volume{
				{Name: `{foo="bar"}`, Volume: 1024},
				{Name: `{bar="baz"}`, Volume: 3350},
			},
			Limit: 5,
		}
		if err := marshal.WriteVolumeResponseJSON(&seriesVolume, w); err != nil {
			serverutil.WriteError(err, w)
			return
		}
	})

@trevorwhitney
Copy link
Collaborator Author

trevorwhitney commented Aug 31, 2023

@liguozhong sorry for the long delay here.

I think there might be some confusion about these endpoints, as the response type from the query frontend is different than the response type from the queriers. The /loki/api/v1/index/volume and /loki/api/v1/index/volume_range endpoints (their current names, changed since this PR was made) return a queryrange.LokiPromResponse (done via this middleware). The queriers do return logproto.Volume responses to the query frontend, and then that middleware merges them together into a queryrange.LokiPromResponse. Since the logcli test is mimicing hitting Loki from the outside, I don't think it should ever be handling a logproto.VolumeResponse since that is a datatype for internal component communication only (ie. queriers -> query frontend).

We are currently using these endpoints in our production cluster, and the payload matches that of the stub in the test, so I think the bug might be in your test setup causes by an incorrect response type?

That being said, I still think this test is valuable to add as is, as we are missing this type of top-level test for logcli.

@trevorwhitney
Copy link
Collaborator Author

it may still be worthwhile to add these tests but this PR is pretty out of date without any action on it for a while

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants