Google-cloud-go: logging: `ListLogEntries` doesn't paginate

[mzhao-va]

ListLogEntries always return all the log entries back no matter what page size I passed in. I tried to hack the library code by hard coding a small page size and it still returns me all items.

Client

cloud.google.com/go/logging

Describe Your Environment

GKE

Expected Behavior

The max number of items that's returned from the api should be the page size

Actual Behavior

Returns all the items at once

[jadekler]

cc @noahdietz could this be a gapic problem?

[noahdietz]

@mzhao-va can you share the version of the libraries you are using?

I'm not able to reproduce this using HEAD. Here is my attempt to repro, please correct me if I'm off. (replace [PROJECT_ID] with an actual one)

package main

import (
    loggingpb "google.golang.org/genproto/googleapis/logging/v2"
    logging "cloud.google.com/go/logging/apiv2"
    "fmt"
    "context"
)

func main() {
    ctx := context.Background()
    c, err := logging.NewClient(ctx)
    if err != nil {
        panic(err)
    }

    req := &loggingpb.ListLogEntriesRequest{
        ResourceNames: []string{"projects/[PROJECT_ID]"},
        PageSize: 5,
    }
    fmt.Printf("Request: %+v\n", req)

    iter := c.ListLogEntries(ctx, req)
    fmt.Printf("Iterator: %+v\n", iter)
    fmt.Printf("Iterator.PageInfo(): %+v\n", iter.PageInfo())

    _, err = iter.Next()
    if err != nil {
        panic(err)
    }
    fmt.Printf("After Next() PageInfo.Remaining(): %d\n", iter.PageInfo().Remaining())


}

The PageInfo reports having the proper PageSize (5 in this case) and after calling Next() to retrieve the page & return the first entry, Remaining() reports having the proper amount left (4 in this case).

[mzhao-va]

We use branch constraint in GoPkg.toml that points at master. And I just did a dep ensure -update and nothing changed. So I believe it's the most recent release.

This is what I did to call ListLogEntries and how I iterate over the results.

        it := ls.loggingClient.ListLogEntries(ctx, resourceNames, filter, pageSize, pageToken)
    var logs []*Log
    for {
        resp, err := it.Next()
        if err == iterator.Done {
            break
        }
        if err != nil {
            return nil, "", err
        }
        logs = append(logs, &Log{
            Timestamp:   resp.GetTimestamp(),
            Severity:    resp.GetSeverity().String(),
            TextPayload: resp.GetTextPayload(),
        })
                 log.Printf("one log returned")
    }
    nextPageToken := it.PageInfo().Token
    log.Printf("pageInfo.maxPage is %d", it.PageInfo().MaxSize))
    log.Printf("pageInfo.remaining() is %d", it.PageInfo().Remaining()))
        return logs, nextPageToken, nil

I'm able to get the correct results of iter.PageInfo().MaxSize and iter.PageInfo().Remaining(). But I still get all the logs back, and the number of times that the printing "one log returned" appears is the number of all the logs.

[noahdietz]

The expected behavior based on what you're saying here (and please correct me if I misunderstand) is that Next() never retrieves more than what you asked for and iterator.Done is returned once you've retrieved PageSize items from the iterator.

So I believe there is a misunderstanding with how the Iterator works, and we might be able to improve our documentation here. @jadekler or @jba please correct me if I butcher this.

Before the first Next() call, the Iterator has no items. Calling Next() retrieves PageSize entries and stores them in an internal buffer, returning the first entry of that page, leaving PageSize-1 entries in the Iterator buffer. It also stores the NextPageToken from this page's response. Once the internal buffer is emptied via PageSize calls to Next() by your program, it will transparently refresh its buffer with the NextPageToken by making a call to the backend, also getting a new NextPageToken. It will do this until there is nothing left. At this point iterator.Done is returned.

Try adding a log.Printf("Next Page Token %s\n", it.PageInfo().Token) to your for loop and you'll see it changing after PageSize calls to Next() are made.

[mzhao-va]

Ohhhh that makes perfect sense. So Next() is going to call stackdriver API multiple times until running out of logs, each time with the pageSize we passed in. Thanks for the explanation!

[mofirouz]

I'd like to wake the dead horse on this issue.

This is an absolutely terribly designed API and it is completely incoherent with what most people expect from what an iterator behaviour should be - and to top this off, making a hidden blocking network request on Next() is one of the worst ideas you could have come up with.

This could be deadly because the iterator is a struct that can be passed around to various locations outside of the context that supposedly made the initial call. This must be made better, and documentation is not enough to fix this.

I expect the iterator to behave like a Java iterator would do. Calling Next() should just return the buffered data which is captured when first making the Stackdriver ListLogEntries call.

[tbpg]

Hi there, @mofirouz. I help to maintain the Go client libraries.

There are many ways to design an iterator, and this is the one we've ended up with here after lots of design review and iteration. There are certainly pros and cons to any design. For guidelines about using this iterator, please see https://github.com/googleapis/google-cloud-go/wiki/Iterator-Guidelines.

There are people behind any given design or piece of software. Criticizing something as one of the worst ideas you could have come up with is neither nice nor productive. I'll also take this time to remind you of the project's code of conduct.

[mofirouz]

Hi @tbpg -

Thanks for your comment and reminding me of code of conduct. While I don't agree that I have abused the code of conduct by my comment above, I do want to apologise to you if my comment has come off impolite.

For the sake of making the developer experience better, It would be great if you could take my comments on the current design of the iterator as community feedback.

[tbpg]

Thanks for understanding. Agreed about not violating the CoC.

Understood. We'll definitely keep this in mind! Let us know if there is something we should clarify in the guidelines above.