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

Improve logql query statistics collection. #1573

Merged
merged 9 commits into from
Jan 24, 2020

Conversation

cyriltovena
Copy link
Contributor

This also add information about ingester queries using grpc trailers.

Next step:

  • Adding this to the all API result.
  • Merging stats in the frontend.
  • Instrument line and byte throughput per query.

Signed-off-by: Cyril Tovena [email protected]

@cyriltovena
Copy link
Contributor Author

cyriltovena commented Jan 24, 2020

benchmark before:

goos: darwin
goarch: amd64
pkg: github.com/grafana/loki/pkg/chunkenc
BenchmarkHeadBlockIterator/Size_100000-16         	     528	   2142509 ns/op	 2400298 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_50000-16          	    1009	   1086106 ns/op	 1204261 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_15000-16          	    4485	    253231 ns/op	  360480 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_10000-16          	    7348	    164947 ns/op	  245792 B/op	       2 allocs/op
PASS
ok  	github.com/grafana/loki/pkg/chunkenc	29.927s

after

goos: darwin
goarch: amd64
pkg: github.com/grafana/loki/pkg/chunkenc
BenchmarkHeadBlockIterator/Size_100000-16         	     513	   2251863 ns/op	 2400296 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_50000-16          	    1060	   1123540 ns/op	 1204259 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_15000-16          	    4336	    260386 ns/op	  360480 B/op	       2 allocs/op
BenchmarkHeadBlockIterator/Size_10000-16          	    7159	    172213 ns/op	  245792 B/op	       2 allocs/op
PASS
ok  	github.com/grafana/loki/pkg/chunkenc	28.987s

This is pretty much similar now ! I had to change a bit the implementation.

This also add information about ingester queries.

Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
Signed-off-by: Cyril Tovena <[email protected]>
}

// Log logs a query statistics result.
func Log(log log.Logger, r Result) {
Copy link
Member

Choose a reason for hiding this comment

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

can you add some comments to these describing what they refer to? I'm having a hard time understandingUncompressed vs Decompressed vs Compressed. I'm guessing one is before decompression, one is after decompression, and one is after recompression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Uncompressed is data that was processed but not compressed from headchunk.
  • Decompressed is data that was processed from decompression store or ingester.
  • Compressed is data that that was decompressed. but the value before decompression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to change the names, or document it better where would it be better to document this ?

@slim-bean ??

Copy link
Member

Choose a reason for hiding this comment

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

I think if you put your comment in the code as a comment it'd be great.

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 thoughts:

Do we really care that they came out of the head chunk? Should we just add the line and byte count to the decompressed counts?

If we do care, i would suggest naming them:

HeadChunkBytes
HeadChunkLines

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

This looks great.

@cyriltovena cyriltovena merged commit f1b8d4d into grafana:master Jan 24, 2020
@cyriltovena cyriltovena deleted the read-stats branch January 24, 2020 20:25
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.

3 participants