Send logs in batches #4378
Send logs in batches #4378
Conversation
adinauer
requested review from
romtsn,
stefanosiano,
markushi and
lcian
as code owners
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Send logs in batches ([#4378](https://github.com/getsentry/sentry-java/pull/4378))If none of the above apply, you can opt out of this check by adding |
| // TODO SentryLevel.TRACE does not exists yet | ||
| // log(SentryLevel.TRACE, message, args); | ||
| // TODO SentryLevel.TRACE does not exists yet so we just report it as DEBUG for now | ||
| log(SentryLevel.DEBUG, message, args); |
There was a problem hiding this comment.
I opted for this as a quick fix to get trace() working. We can have a proper fix later but we'll have to check what consequences this has for the rest of the SDK since the enum is used in multiple places already.
| traceContext = propagationContext.traceContext(); | ||
| } | ||
| } | ||
| // @Nullable TraceContext traceContext = null; |
There was a problem hiding this comment.
(maybe) Updating baggage should be done in LoggerApi to freeze it on the first event going out.
There was a problem hiding this comment.
I'm not sure, I don't fully understand when it should be done.
| @Override | ||
| public void add(final @NotNull SentryLogEvent logEvent) { | ||
| queue.offer(logEvent); | ||
| maybeSchedule(false, false); |
There was a problem hiding this comment.
This means we're locking on every log event. I'll try to optimize this. Ideas:
- Have a bool
hasScheduledor similar which we check first and avoid locking if already true - Check the futures state first (not sure about performance of that)
- Just send a signal and have a background thread pick it up by periodically polling for it
Performance metrics 🚀
|
| try { | ||
| final @NotNull SentryEnvelope envelope = | ||
| buildEnvelope(new SentryLogEvents(Arrays.asList(logEvent)), traceContext); | ||
| hint.clear(); |
There was a problem hiding this comment.
Before we were passing the hint to sendEnvelope, now we always use null as the hint.
Does it make any difference?
There was a problem hiding this comment.
Yeah it might have some effect especially for Android but I haven't found a good way to pass the hint through the batch processor unless we hold on to it until it's been sent and even then which Hint should we use if there's 100 log events being batched together?
| traceContext = propagationContext.traceContext(); | ||
| } | ||
| } | ||
| // @Nullable TraceContext traceContext = null; |
There was a problem hiding this comment.
I'm not sure, I don't fully understand when it should be done.
| flushBatch(); | ||
| if (queue.size() >= MAX_BATCH_SIZE) { | ||
| flushInternal(); | ||
| } |
There was a problem hiding this comment.
There is no need for recursion, personally I would prefer
| flushBatch(); | |
| if (queue.size() >= MAX_BATCH_SIZE) { | |
| flushInternal(); | |
| } | |
| while (queue.size() >= MAX_BATCH_SIZE) { | |
| flushBatch(); | |
| } |
There was a problem hiding this comment.
This would mean we send out at most two batches at a time. The way it's implemented atm sends out as many full batches as are available and then maybe reschedules (depending on queue being empty).
There was a problem hiding this comment.
Ah I missed the while, sorry
There was a problem hiding this comment.
But needs to be do ... while so we send out before having a full batch
| } | ||
|
|
||
| @Override | ||
| public void close(final boolean isRestarting) { |
There was a problem hiding this comment.
just wondering, when is isRestarting intended to be used?
There was a problem hiding this comment.
When the SDK is being re-initialized. LMK if you need more explanation.
📜 Description
Instead of sending each log event on its own immediately, we now batch them and send after 5s.
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps