open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
850 stars 403 forks source link

[API] Logger::EmitLogRecord() is unsafe to use #2666

Closed marcalff closed 4 months ago

marcalff commented 4 months ago

Consider the following code:

        nostd::unique_ptr<opentelemetry::logs::LogRecord> record =
            logger->CreateLogRecord();
        record->SetSeverity(otel_severity);
        opentelemetry::common::AttributeValue otel_body(message);
        record->SetBody(otel_body);
        logger->EmitLogRecord(record);

This code builds, executes, and produces empty log records downstream.

The issue is that this method is invoked:

  template <class... ArgumentType>
  void EmitLogRecord(ArgumentType &&... args)
  {
    nostd::unique_ptr<LogRecord> log_record = CreateLogRecord();
    if (!log_record)
    {
      return;
    }

    EmitLogRecord(std::move(log_record), std::forward<ArgumentType>(args)...);
  }

The log record given in arguments is ignored, and a dummy record, not populated, is used instead.

To fix this, the user code needs to change to:

        logger->EmitLogRecord(std::move(record));

which will invoke:

  virtual void EmitLogRecord(nostd::unique_ptr<LogRecord> &&log_record) noexcept = 0;

This is a major issue, because a lot of user code will be instrumented for logs, and these mistakes will happen again and again, creating the perception that opentelemetry-cpp does not work for logs.

Code such as:

        logger->EmitLogRecord(record);

MUST fail to build instead of ignoring the record argument.

owent commented 4 months ago

Good point, I can take this several days later when I have time. We can deny unknown types or some well-known types in type traits.