Use of unsafe Code for performance and capabilities

[lalitb]

I believe, there is an undocumented understanding among contributors of this repo not to use unsafe code. However, we can allow it on a case-by-case basis, given the potential benefits in performance and capabilities, with assurance that it won't compromise the memory safety.

• Not using unsafe keywords poses many limitations in implementing lock-free capabilities. The atomic locks are only supported for primitive types, and any support for more complex types needs to retrieve the raw pointer of that object and lock using AtomicPtr. However, any such pointer operation can only be done using unsafe code.
• As an example, we did achieve a 10x boost in the throughput Metrics Aggregation - Improve throughput by 10x #1833 in the hot path of metrics recording for sum aggregation. This was possible because the aggregation value is Number, which can be updated atomically in hashmap under RWLock. However, it won't be easy to achieve the same with Histogram aggregation with more complex aggregated values, without unsafe keyword.
• If we plan to replace the async-runtime channels used in batch processors with (say) lock-free CircularBuffer, it won't be feasible to achieve it without unsafe.
• The alternative could be to use some lock-free hashmap or circular-buffer implementation through some external crate (which would be internally using unsafe), but I feel it would be safer to have control of such data structures within our repo.
• The tracing library(which is more lower in stack) uses the unsafe keywork when required. So, the end-users cannot impose restrictions on themselves against using crates with unsafe code for their telemetry pipeline.

It is not feasible to implement #1943 or #1942 (adding a shared LogRecord pool), so wanted to have a discussion here before further deciding on the design.

ps. We do use unsafe at one place in opentelemetry-http but I believe it is possible to remove that if we decide against using unsafe.

[shaun-cox]

I would just caution us to resist the urge to think that the route to faster performance is ideally started by taking a left turn at unsafe/safe before considering the shared-nothing/shared or single-thread/multi-thread avenues first.

[lalitb]

Thanks @shaun-cox. This is a fair ask. That's the reason I mentioned it as case-to-case basis :

• once all the safe options (eg thread-local cache, shared-nothing for lock-free caches) have been evaluated. And unsafe as the last resort.
• Have unsafe option only with thorough vetting to ensure we don't compromise safety.

[lalitb]

And then there are some scenarios where it becomes unavoidable. For example, when interfacing with a C API, such as in my PR in the contrib repository for a journald exporter, which interfaces with libsystemd and cannot be done without using unsafe.

[cijothomas]

+1 to Shaun's wording. I'd consider unsafe as the true last resort, evaluated on a case to case basis.

If we plan to replace the async-runtime channels used in batch processors with (say) lock-free CircularBuffer, it won't be feasible to achieve it without unsafe.

For specific scenarios like this, please create specific issues to track them, and get feedback on why is unsafe the only way to solve it.

[cijothomas]

As an example, we did achieve a 10x boost in the throughput #1833 in the hot path of metrics recording for sum aggregation. This was possible because the aggregation value is Number, which can be updated atomically in hashmap under RWLock. However, it won't be easy to achieve the same with Histogram aggregation with more complex aggregated values, without unsafe keyword.

The gains are coming not purely from use of "atomic" instructions. For example, the f64 version of Number is implemented with plain Mutex locks: https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-sdk/src/metrics/internal/mod.rs#L136-L137