Skip to content

Add target filtering for logs#37

Merged
byronjinmsft merged 6 commits intomainfrom
users/ravakella/add-target-filter-for-logs
Aug 11, 2025
Merged

Add target filtering for logs#37
byronjinmsft merged 6 commits intomainfrom
users/ravakella/add-target-filter-for-logs

Conversation

@ravakella-ms
Copy link
Contributor

@ravakella-ms ravakella-ms commented Aug 4, 2025

Adding a target_filter attribute for LogExportTargets struct. This will help users attach a target to their logs and make sure only a subset of the logs are going to be exported to the Otel collector

Unrelated to the main feature, there is a make test failure in mocks.rs. The [clippy::result_large_err] lint warns when the [Err] variant of a Result<T, E> type is very large (in this case, 176 bytes). This can be problematic because:

  • Memory Usage: Result<T, E> is stored as a tagged union, so it takes up space equal to the larger of T or E plus some overhead for the discriminant tag.
  • Performance: Large error types can impact performance during:
    Stack allocation/deallocation, Moving/copying the Result around, etc

Why is tonic::Status Large?
The tonic::Status type is large because it contains:
Error code, Error message (String), Error details, Metadata, Source information. All of this adds up to 176+ bytes.
But since this code is:

  • Test/mock code (not production)
  • A simple interceptor that rarely fails
  • Following the tonic interceptor API signature
    The #[allow(clippy::result_large_err)] is reasonable here because:
  • We can't easily change the API (it's defined by tonic)
  • The performance impact is minimal in tests
  • Boxing the error would add unnecessary heap allocation for test code

@ravakella-ms ravakella-ms changed the title Users/ravakella/add target filter for logs Add target filter for logs Aug 11, 2025
@ravakella-ms ravakella-ms changed the title Add target filter for logs Add target filtering for logs Aug 11, 2025
@byronjinmsft byronjinmsft self-requested a review August 11, 2025 22:52
@byronjinmsft byronjinmsft merged commit 7917321 into main Aug 11, 2025
4 checks passed
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.

3 participants