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

New fallback sink on failed exports #156

Closed
wants to merge 8 commits into from
Closed

New fallback sink on failed exports #156

wants to merge 8 commits into from

Conversation

alsi-lawr
Copy link
Contributor

@alsi-lawr alsi-lawr commented Aug 21, 2024

Disclaimer: I used ChatGPT to help with writing parts of this summary.

Summary

We introduce a new robust fallback mechanism to the sink, allowing logs and traces to be saved locally when the primary export to OpenTelemetry fails. This enhancement ensures greater resilience and data integrity by capturing logs and traces that would otherwise be lost during export failures.

Publicly visible changes

  • Fallback Configuration:

    • Added FallbackConfigurationOptions to allow users to configure fallback paths for logs and traces separately or using a unified fallback. This configuration is part of the OpenTelemetrySinkOptions.
    • Users can specify fallback paths for logs (LogFallback), traces (TraceFallback), and a general fallback (Fallback) if specific configurations are not enabled.
  • Support for NDJSON and Protobuf Formats:

    • The fallback mechanism supports saving logs and traces in both newline-delimited JSON and delimited Protobuf formats. This provides flexibility depending on the preferred storage and processing format.
  • New Tests and Validation:

    • Added unit tests to validate the fallback mechanism, ensuring that logs and traces are correctly saved to the specified paths when the export fails.
    • The tests cover both JSON and Protobuf formats, as well as various scenarios for logs, traces, and general fallbacks.
  • Documentation and Examples:

    • Updated documentation to instruct users on how to configure the fallback mechanism.
    • Included examples demonstrating the configuration and usage of the fallback options.

Internal changes

  • Exporters now return ExportResults. Consumption of these reults has been handled with a functional approach using deferred execution pipelines to fully capture failures and prevent thread-blocking behaviour.
  • A new dependency on the Serilog.Sinks.File package was necessarily introduced to prevent duplicated efforts.
  • The file formatter disregards the LogEvent and instead grabs the actual transformed OTLP message from a LogEvent property, as we're using the File sink as a redirect mechanism and not a real logger.

Benefits

  • Increased Resilience: Logs and traces are preserved even if the OpenTelemetry export fails, ensuring that critical data is not lost.
  • Flexibility: Users can configure separate fallbacks for logs and traces or use a unified fallback configuration.
  • Compatibility: The changes are fully compatible with existing configurations, allowing for easy integration into current setups.

Resolved Issues

@alsi-lawr alsi-lawr marked this pull request as draft August 22, 2024 11:49
@alsi-lawr
Copy link
Contributor Author

alsi-lawr commented Aug 22, 2024

Converted to draft as this can be extended to be much more flexible.
Edit: Reopened as final draft implementation

@alsi-lawr alsi-lawr marked this pull request as ready for review August 22, 2024 19:26
@nblumhardt
Copy link
Member

Thanks for all the additional feedback and exploration!

Adding a direct dependency on a fallback sink (such as File) is probably a non-starter at this point because of past experiences maintaining and supporting the same concept (e.g. in the Seq and Elastic sinks, among others).

If we move forward with support for fallback sinks via the Serilog failure listener feature, the implementation will be unlikely to bear any similarity to this, because the functionality will be cross-cutting and not per-target-sink. It might be best to close this PR and keep discussion on the linked ticket.

Thanks again!

@alsi-lawr alsi-lawr closed this Aug 27, 2024
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