fix: do not stop sampling processor when failing to delete trace events

[kruskall]

Motivation/summary

The sampling processor should never stop when apm-server is running. Instead log an error on Warn level and skip the current event.

Handle ErrTxnTooBig error when deleting trace events: because deleting adds a transaction it can increase the size above the limit.
Flush the events before deleting.

Checklist

• Update CHANGELOG.asciidoc
• Documentation has been updated

For functional changes, consider:

• Is it observable through the addition of either logging or metrics?
• Is its use being published in telemetry to enable product improvement?
• Have system tests been added to avoid regression?

How to test these changes

Related issues

Closes #12053

[mergify]

This pull request does not have a backport label. Could you fix it @kruskall? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-7.17 is the label to automatically backport to the 7.17 branch.
• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.

NOTE: backport-skip has been added to this pull request.

[simitt]

From the issue description:

To fix this we need to

already reject events that exceed a max size when they are received, as well as ensuring that events of allowed size can be processed by the system

This PR addresses the part of the github issue to not stop the TBS processor, but it does not address the problem that events exceeding the max size fill up storage without ever getting deleted. Events that are too large to be deleted, should be rejected already when received.

[carsonip]

already reject events that exceed a max size when they are received

There is the same ErrTxnTooBig check in writeEntry, so we should already be doing it.

Did the deletion txn get too large because writes are batched? Should we handle the ErrTxnTooBig thrown by flushing and retrying?

[simitt]

There is the same ErrTxnTooBig check in writeEntry, so we should already be doing it.

The code ensures that an event is added to a new transaction if the max size of the write transaction would otherwise be exceeded. I overlooked that the event cannot be stored in the first place if the event itself is exceeding the max transaction size.

Did the deletion txn get too large because writes are batched? Should we handle the ErrTxnTooBig thrown by flushing and retrying?

Yes, that sounds like the only reason why this might fail. +1 on the suggested solution.

[kruskall]

This PR addresses the part of the github issue to not stop the TBS processor, but it does not address the problem that events exceeding the max size fill up storage without ever getting deleted.

There is the same ErrTxnTooBig check in writeEntry, so we should already be doing it.

Yep, we're keeping track of buffered writes so the storage shouldn't become too big as opposed to the previous approach.

Did the deletion txn get too large because writes are batched? Should we handle the ErrTxnTooBig thrown by flushing and retrying?

I'm not sure what this means, can you elaborate ?

[carsonip]

I'm not sure what this means, can you elaborate ?

DeleteTraceEvent and WriteTraceEvent share the same rw. Imagine processor calls WriteTraceEvent multiple times right before reaching ErrTxnTooBig, then calls DeleteTraceEvent, which takes the txn right over the txn size limit. Since there is no ErrTxnTooBig handling inside DeleteTraceEvent unlike writeEntry, it will throw an error. I suspect this is the root cause.

To fix it, handle ErrTxnTooBig gracefully in DeleteTraceEvent like what we have in writeEntry

[kruskall]

Ah right, deleting adds a new entry 🤦

[carsonip]

lgtm

• Can you update the PR description?
• Do we want to backport this to 8.12, i.e. 8.12.2?

[kruskall]

Can you update the PR description?

Done

Do we want to backport this to 8.12, i.e. 8.12.2?

I guess we could do that since we did the same for the other tbs fix. cc @simitt who opened the issue for confirmation

[simitt]

It's a small enough fix, so no objections to backport to 8.12.2.

[kruskall]

@mergify backport 8.12

[mergify]

backport 8.12

✅ Backports have been created

• #12664 fix: do not stop sampling processor when failing to delete trace events (backport #12509) has been created for branch 8.12

[carsonip]

Testing notes

✔️ test-plan-ok

Tested via #12688 as it is almost impossible to manually test this behavior. Test in #12688 fails without the handling in this PR.