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

Buffer custom logs until a service connection is established #49

Merged
merged 18 commits into from
Apr 12, 2022

Conversation

jmatsu
Copy link
Contributor

@jmatsu jmatsu commented Apr 9, 2022

ref: #48

It may be better to buffer all instructions, but we have decided to start addressing this issue from the custom logging.

Approach

  • Use Handler to control instructions for logging
  • Buffering
    • Buffer logs if a service connection is unavailable
    • Start sending logs when a service connection is established
    • Default backpressure is Drop By Oldest (similar to the current behavior)
  • Performance-aspect
    • Any processing doesn't work if the buffer is empty
    • Do not poll the buffer
    • Do not get locks as much as possible
  • Fix a bug in the current master

Do in another PR

  • Measure wasted-time in the buffer

Overview of custom log buffering

sequenceDiagram
    participant Android
    participant UserApp
    participant SDK
    participant ClientApp
opt DirectBoot
    Android->>UserApp: Direct boot event
    UserApp->>SDK: Log a message
    SDK-->SDK: Store the log
end
opt NormalBoot
    Android->>UserApp: Boot completed event
    UserApp->>SDK: Log a message
    SDK-->SDK: Store the log
end

Android->>ClientApp: Boot completed
ClientApp->>SDK: Service connection is established

par Process buffered logs
    SDK->>ClientApp: Send buffered logs
and Process a new fresh log
    UserApp->>SDK: Log a message
    SDK-->ClientApp: Send it
end

loop Process a new log
    UserApp->>SDK: Log a message
    SDK-->ClientApp: Send it
end
Loading

Rejected idea

Polling

  • ✅ Easy to keep sending buffered logs
  • ❌ Polling costs would be high

Concurrent Deque or else

  • ✅ Better performance
  • ❌ need to maintain the thread-safe implementation

@jmatsu jmatsu force-pushed the feat/send_logs_with_buffering branch from 0f0e120 to a1e1191 Compare April 10, 2022 11:03
@jmatsu jmatsu marked this pull request as ready for review April 11, 2022 08:49
@jmatsu jmatsu force-pushed the feat/send_logs_with_buffering branch from 4cbbf23 to 7257cf1 Compare April 11, 2022 09:55
@jmatsu jmatsu force-pushed the feat/send_logs_with_buffering branch from 7257cf1 to 2ea725f Compare April 11, 2022 10:17
@jmatsu jmatsu requested a review from tnj April 11, 2022 15:07
Copy link
Member

@tnj tnj left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, with minor confirmations and corrections commented

@jmatsu jmatsu requested a review from tnj April 12, 2022 07:08
@jmatsu
Copy link
Contributor Author

jmatsu commented Apr 12, 2022

@tnj could you please review this again?

Copy link
Member

@tnj tnj left a comment

Choose a reason for hiding this comment

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

LGTM!

@jmatsu
Copy link
Contributor Author

jmatsu commented Apr 12, 2022

Thank you so much! 🎉

@jmatsu jmatsu merged commit 84156c6 into master Apr 12, 2022
@jmatsu jmatsu deleted the feat/send_logs_with_buffering branch April 12, 2022 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants