Skip to content

Add tests for shipper output and add support for partial results#31558

Merged
rdner merged 7 commits intoelastic:mainfrom
rdner:shipper-output-tests
May 11, 2022
Merged

Add tests for shipper output and add support for partial results#31558
rdner merged 7 commits intoelastic:mainfrom
rdner:shipper-output-tests

Conversation

@rdner
Copy link
Copy Markdown
Member

@rdner rdner commented May 9, 2022 •
edited
Loading

An event batch can be partially accepted by the server.

Added test cases for:

  • An event batch is published from an input to the shipper gRPC server
  • An event batch is not dropped when the gRPC server is not available but starts later
  • An event batch is not dropped when the shipper's queue is exhausted and the events are accepted only partially

What does this PR do?

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
    - [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • An event batch is published from an input to the shipper gRPC server
  • An event batch is not dropped when the gRPC server is not available but starts later
  • An event batch is not dropped when the shipper's queue is exhausted and the events are accepted only partially

How to test this PR locally

Related issues

@rdner
Add tests for shipper output and add support for partial results
9bf2c12 
An event batch can be partially accepted by the server.

Added test cases for:

* An event batch is published from an input to the shipper gRPC server
* An event batch is not dropped when the gRPC server is not available but starts later
* An event batch is not dropped when ResourceExhausted code is returned from the gRPC
@rdner rdner self-assigned this May 9, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 9, 2022
rdner
rdner commented May 9, 2022
View reviewed changes
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 9, 2022 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-11T12:00:19.459+0000

  • Duration: 68 min 54 sec

Test stats 🧪

Test Results
Failed 0
Passed 22596
Skipped 1934
Total 24530

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented May 10, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b shipper-output-tests upstream/shipper-output-tests
git merge upstream/main
git push upstream shipper-output-tests

@rdner rdner marked this pull request as ready for review May 10, 2022 15:18
@rdner rdner requested a review from a team as a code owner May 10, 2022 15:18
@rdner rdner requested review from belimawr, faec and kvch and removed request for a team May 10, 2022 15:18
@rdner rdner added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label May 10, 2022
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 10, 2022

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 10, 2022
@rdner rdner requested a review from cmacknz May 10, 2022 15:19
cmacknz
cmacknz reviewed May 10, 2022
View reviewed changes
belimawr
belimawr reviewed May 10, 2022
View reviewed changes
cmacknz
cmacknz reviewed May 10, 2022
View reviewed changes
@rdner rdner requested review from belimawr and cmacknz May 11, 2022 12:26
cmacknz
cmacknz reviewed May 11, 2022
View reviewed changes
libbeat/outputs/shipper/shipper.go
},
})

nonDroppedEvents = append(nonDroppedEvents, e)
Copy link
Copy Markdown
Member

@cmacknz cmacknz May 11, 2022

Choose a reason for hiding this comment

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

The publisher.Event type is a struct, not a reference:

beats/libbeat/publisher/event.go

Line 50 in 47777ec

type Event struct {

We want to make sure we aren't making a copy of every event here unnecessarily. Could you keep a reference, or just the index in the array instead?

cmacknz
cmacknz reviewed May 11, 2022
View reviewed changes
libbeat/outputs/shipper/shipper.go
batch.Retry()
}
return fmt.Errorf("failed to publish the batch to the shipper: %w", err)
if status.Code(err) != codes.OK || resp == nil {
Copy link
Copy Markdown
Member

@cmacknz cmacknz May 11, 2022

Choose a reason for hiding this comment

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

If the shipper isn't available briefly, because of an upgrade or a crash for example, what happens to the events in this case. The batch is cancelled, but does the rest of the pipeline retry?

Copy link
Copy Markdown
Member Author

@rdner rdner May 11, 2022

Choose a reason for hiding this comment

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

Good question, I assumed that the pipeline would try again in any case if the batch is cancelled. That's what I understood from the function's doc here:

beats/libbeat/publisher/event.go

Lines 44 to 45 in dc4933d

// Send was aborted, try again but don't decrease the batch's TTL counter.
Cancelled()

@rdner rdner merged commit 8a3f1dc into elastic:main May 11, 2022
@rdner rdner deleted the shipper-output-tests branch May 11, 2022 19:24
@rdner rdner mentioned this pull request Jun 21, 2022
Merged
4 tasks
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
@rdner @chrisberkhout
Add tests for shipper output and add support for partial results (#31558
e0ab3d7 
)

An event batch can be partially accepted by the server.

Added test cases for:

* An event batch is published from an input to the shipper gRPC server
* An event batch is not dropped when the gRPC server is not available but starts later
* An event batch is not dropped when ResourceExhausted code is returned from the gRPC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmacknz cmacknz cmacknz approved these changes

@kvch kvch Awaiting requested review from kvch kvch is a code owner automatically assigned from elastic/elastic-agent-data-plane

@faec faec Awaiting requested review from faec

@belimawr belimawr Awaiting requested review from belimawr

Assignees

@rdner rdner

Labels

enhancement Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team v8.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add integration tests for the shipper output

4 participants

@rdner @elasticmachine @belimawr @cmacknz