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

[Logs SDK] Rename and update existing LogRecord class, modify existing Recordable interface #1691

Closed
lalitb opened this issue Oct 17, 2022 · 2 comments · Fixed by #1766
Closed

Comments

@lalitb
Copy link
Member

lalitb commented Oct 17, 2022

  • To avoid conflict with the new LogRecord class, rename existing sdk::logs::LogRecord class to some better name (say sdk::logs::LogRecordData).
  • Add setter for observed_timestamp in sdk:logs::Reordable and sdk::logs::LogRecordData.
  • Similar to Span's Recordable, add a getter method for LogRecordData in Recordable class. This can be eventually implemented by all it's derived implementation (not in scope for now).
  /**
   * Get the LogRecordData object for this Recordable.
   *
   * @return LogRecordData*
   */

  virtual explicit operator LogRecordData *() const { return nullptr; }

This would let us avoid having ReadWriteLogRecord class. This method can be potentially used inside LogRecordProcessor to filter the logs based on it's contents (not in scope for now).

@marcalff marcalff added the logs label Oct 17, 2022
@lalitb lalitb changed the title [Logs SDK] Rename and update existing LogsRecord class, modify existing Recordable interface [Logs SDK] Rename and update existing LogRecord class, modify existing Recordable interface Oct 17, 2022
@owent
Copy link
Member

owent commented Oct 18, 2022

Can we just rename it to ReadWriteLogRecord and just set data of Recordable from it?

// API:
namespace logs{
// prune class which only contains setters
class LogRecord;
}

//SDK
namespace sdk {
namespace logs{
// prune class which contains getters
class ReadableLogRecord : public logs::LogRecord;

// A replacement of legacy LogRecord
class ReadWriteLogRecord : public LogRecord;

class Recordable {
public:
  void SetLogRecord(const ReadableLogRecord & record) {
    SetTimestamp(record.GetTimestamp());
    SetSeverity(record.GetSeverity());
    SetBody(record.GetBody());
   // And etc.
  }

  virtual void SetTimestamp(opentelemetry::common::SystemTimestamp timestamp) noexcept = 0;
  virtual void SetSeverity(opentelemetry::logs::Severity severity) noexcept = 0;
  virtual void SetBody(nostd::string_view message) noexcept = 0;
  // etc.
};
}
}

How about this design? I think it's more simular to the relationship of trace span and recordable.

@lalitb
Copy link
Member Author

lalitb commented Oct 19, 2022

@owent - Yes, that's look good too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants