Skip to content

fix: empty the write buffer before exit#465

Merged
vigneshshanmugam merged 2 commits intoelastic:mainfrom
vigneshshanmugam:write-all-data
Mar 8, 2022
Merged

fix: empty the write buffer before exit#465
vigneshshanmugam merged 2 commits intoelastic:mainfrom
vigneshshanmugam:write-all-data

Conversation

@vigneshshanmugam
Copy link
Copy Markdown
Member

@vigneshshanmugam vigneshshanmugam commented Mar 5, 2022 •
edited
Loading

  • fix reporter stream buffer is not emptied in all cases #446

  • The max buffer capacity for SonicBoom is 64 * 1024 bytes and the API flush and flushSync behaves differently when it comes to writing the last data from the streams buffer.

  • flush API only writes the current element in the buffer which is always the first item and drops other data and closes the FD.

  • However flushSync ensures all the data from the buffer is completely written.

Example

// Previous - FLUSH Async
SONICBOOM: Data-0-65189
SONICBOOM: Data-1-29862
SONICBOOM: flush-95051
SONICBOOM: Writing-65189
-- Data 1 is completetly lost
- Runner closed

// Current - FlushSync
SONICBOOM: Data-0-61310
SONICBOOM: Data-1-53290
SONICBOOM: flushSync-114600
SONICBOOM: Writing-61310
SONICBOOM: Writing-53290
- Runner closed

@ghost
Copy link
Copy Markdown

ghost commented Mar 5, 2022 •
edited by ghost
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-03-05T04:05:38.652+0000

  • Duration: 14 min 10 sec

Test stats 🧪

Test Results
Failed 0
Passed 148
Skipped 2
Total 150

🤖 GitHub comments

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

  • /test : Re-trigger the build.

vigneshshanmugam
vigneshshanmugam commented Mar 8, 2022
View reviewed changes
__tests__/core/runner.test.ts
* FLAKY skipped
*/
it.skip('run - ensure journey/end is written for real world pages', async () => {
it('run - ensure journey/end is written for real world pages', async () => {
Copy link
Copy Markdown
Member Author

@vigneshshanmugam vigneshshanmugam Mar 8, 2022

Choose a reason for hiding this comment

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

We cant get a better indicator than this test, I am keeping this as it is.

I did not want to test the sonic-boom package functionality in our agent.

dominiqueclarke
dominiqueclarke approved these changes Mar 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@dominiqueclarke dominiqueclarke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. Working as expected with Heartbeat 8.2.0 and test script.

@vigneshshanmugam vigneshshanmugam merged commit 41330fd into elastic:main Mar 8, 2022
@vigneshshanmugam vigneshshanmugam deleted the write-all-data branch March 8, 2022 17:19
@lucasfcosta lucasfcosta mentioned this pull request Mar 15, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dominiqueclarke dominiqueclarke dominiqueclarke approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

reporter stream buffer is not emptied in all cases

2 participants

@vigneshshanmugam @dominiqueclarke