[ENH] Do not trace failed preconditions when flushing compaction.

[rescrv]

Description of changes

The soft-deleted compaction will return an error that's not
distinguishable from other errors. It will percolate to the dispatcher
and be logged like every other error.

In #5064, we added a should_trace_error function on the chroma Error
trait, which will silence the error at the dispatcher. This PR applies
that annotation to the failed precondition.

Test plan

CI

Migration plan

N/A

Observability plan

We get paged today.

We don't get paged in the future.

Documentation Changes

N/A

[propel-code-bot]

This PR updates the FlushCompactionError implementation so that failed precondition errors-typically raised in cases like soft-deleted compactions-are not traced as errors in the dispatcher, reducing unnecessary logging and paging. This leverages the recently added should_trace_error function in the ChromaError trait to control which errors are surfaced for tracing.

This summary was automatically generated by @propel-code-bot

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[propel-code-bot]

[BestPractice]

You can simplify the type conversion here. The compiler can infer the type for .into() from the comparison with ErrorCodes::FailedPrecondition, making the explicit type annotation unnecessary.

Suggested change

	FlushCompactionError::FailedToFlushCompaction(status) => {
	if std::convert::Into::<chroma_error::ErrorCodes>::into(status.code())
	== ErrorCodes::FailedPrecondition
	{
	ErrorCodes::FailedPrecondition
	} else {
	ErrorCodes::Internal
	}
	}
	FlushCompactionError::FailedToFlushCompaction(status) => {
	if status.code().into() == ErrorCodes::FailedPrecondition {
	ErrorCodes::FailedPrecondition
	} else {
	ErrorCodes::Internal
	}
	}

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

[codetheweb]

I didn't see should_trace_error and honestly am having a little trouble understanding it. In my mind there are only two cases: either the result is an error and we should error!/trace, or the result is something we expect to happen under normal conditions. In other words, I'm not quite getting why we push an idea of "nominal"/expected error conditions down into the trait.

[rescrv]

I added should_trace_error a few weeks ago.

The core problem is that the system crate is oblivious to the nature of higher level errors, taking anything that fits in the return value as an error. I don't see an easy way to make it so that we can return a failure without an error, which is what we want. should_trace_error is the way to do that. Every time someone wants to log an error from a chroma error they can and they benefit from the author's interpretation of which errors were traced. The only other ways I know to do that are manual coding and language features rust doesn't support easily. A trait method is the right way.

In general I disagree with tracing every error, but that's not for this PR.