[API] NoopLogger causes segfault

[yurishkuro]

trafficstars

Describe your environment v1.14.2

Steps to reproduce

#include "opentelemetry/logs/provider.h"

int main(int argc, char* argv[]) {
  auto logs_provider = opentelemetry::v1::logs::Provider::GetLoggerProvider();
  auto logger = logs_provider->GetLogger("test");
  auto sample = logger->CreateLogRecord();
  sample->SetAttribute("some key", "some value");
  logger->EmitLogRecord(std::move(sample));
  return 0;
}

What is the expected behavior? Program should run successfully.

What is the actual behavior? SEGV at sample->SetAttribute because sample is null.

Additional context https://github.com/open-telemetry/opentelemetry-cpp/blob/da8e377be51edadad514d31c58e0ce7a2eb1b05e/api/include/opentelemetry/logs/noop.h#L37

The NoopLogger is supposed to return usable objects, not a nullptr.

[lalitb]

The NoopLogger is supposed to return usable objects, not a nullptr.

Good point. It should indeed return NoopLogRecord.

[yurishkuro]

One other challenge when I tried to implement a solution is that the nostd::unique_ptr class does not support deleter overrides, which prevents me from having the CreateLogRecord() function always return the same static instance of no-op record.

class NoopLogger final : public Logger
{
private:
  class NoopLogRecord : public LogRecord{
  public:
    void SetTimestamp(common::SystemTimestamp timestamp) noexcept override {};
    void SetObservedTimestamp(common::SystemTimestamp timestamp) noexcept override {};
    void SetSeverity(logs::Severity severity) noexcept override {};
    void SetBody(const common::AttributeValue &message) noexcept override {};
    void SetAttribute(nostd::string_view key,
                              const common::AttributeValue &value) noexcept override {};
    void SetEventId(int64_t id, nostd::string_view name = {}) noexcept override {};
    void SetTraceId(const trace::TraceId &trace_id) noexcept override {};
    void SetSpanId(const trace::SpanId &span_id) noexcept override {};
    void SetTraceFlags(const trace::TraceFlags &trace_flags) noexcept override {};
  };

public:
  const nostd::string_view GetName() noexcept override { return "noop logger"; }

  nostd::unique_ptr<LogRecord> CreateLogRecord() noexcept override {
    // always creates a new copy of noop log record
    return nostd::unique_ptr<LogRecord>(new NoopLogRecord());
  }
...

[marcalff]

A possible way is to implement an operator new() and operator delete() for the NoopLogRecord, that will return a static dummy on new.

I agree it causes a lot of convoluted code for something that should have been very simple, but avoiding a (real) new + delete on every noop logger is worth it.

[yurishkuro]

https://github.com/open-telemetry/opentelemetry-cpp/pull/2662

This change ^ worked in my local internal setup (where we import OTEL into monorepo). However the other tests are failing, I am not able to debug them right now since I do not have the oss version of the build setup.