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.

Sign up for GitHub

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

Jump to bottom

[API] NoopLogger causes segfault #2656

Closed
yurishkuro opened this issue May 6, 2024 · 4 comments · Fixed by #2668
Closed

[API] NoopLogger causes segfault #2656

yurishkuro opened this issue May 6, 2024 · 4 comments · Fixed by #2668
Assignees
@marcalff
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@yurishkuro
Copy link
Member

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

opentelemetry-cpp/api/include/opentelemetry/logs/noop.h

Line 37 in da8e377

nostd::unique_ptr<LogRecord> CreateLogRecord() noexcept override { return nullptr; }

The NoopLogger is supposed to return usable objects, not a nullptr.
@yurishkuro yurishkuro added the bug Something isn't working label May 6, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label May 6, 2024
@lalitb
Copy link
Member

lalitb commented May 6, 2024

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

Good point. It should indeed return NoopLogRecord.

@marcalff marcalff changed the title NoopLogger causes segfault [API] NoopLogger causes segfault May 6, 2024
@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 6, 2024
@yurishkuro
Copy link
Member Author

yurishkuro commented May 6, 2024 •
edited

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
Copy link
Member

marcalff commented May 7, 2024 •
edited

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
Copy link
Member Author

#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.

@marcalff marcalff mentioned this issue May 7, 2024
Closed
3 tasks
marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue May 14, 2024
@marcalff
Fixes open-telemetry#2656
e99e383
@marcalff marcalff mentioned this issue May 14, 2024
Merged
3 tasks
@marcalff marcalff assigned marcalff and unassigned ThomsonTan May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marcalff marcalff

Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
No milestone
4 participants
@ThomsonTan @lalitb @yurishkuro @marcalff