Review the changes of logs specification

[owent]

Hi all @open-telemetry/cpp-approvers . I have reviewed the changes of log specification recently and found there are a lot changes from #1291 . I think we should discuss the detail of implementation about these changes before I start to write the codes.

First, I can rarely attend the meeting because the meeting time and timezone.(13:00 PT and 09:00PT are 04:00 AM and 00:00AM in my timezone). Sorry for that and the meeting time on wednesday has be moved up to 09:00PT(which is 00:00AM in my timezone) maybe I
can attend this one sometimes.

To my understanding, the changes are listed below:

• API: (According to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/api.md)
  • event_domain should be added into LoggerProvider::GetLogger()
  • attributes should also be added into LoggerProvider::GetLogger(), should we merge the attributes from LoggerProvider::GetLogger and Logger::Log() before we exporting it to processor?
  • include_trace_context should also be added into LoggerProvider::GetLogger(), but if user do not use trace::Scope , we can not get the current Span from RuntimeContext. Should we keep to use trace data from Logger::Log() or just use get Span from RuntimeContext ?
  • api::logs::LogRecord should be added into API to replace the logs::Recordable in SDK.
  • Add prune Logger::Emit(LogRecord, {event name, attributes and timestamp}) to replace prune Logger::Log(). We can keep the existing Logger::Log() to call Logger::Emit.
• SDK: (According to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/api.md)
  • OnReceive should be renamed to OnEmit ( Rename OnReceive to OnEmit #1652 )
  • We could add sdk::logs::ReadWriteLogRecord to inherit api::logs::LogRecord.I found the processor of otel-java SDK only emit ReadWriteLogRecord, maybe we can also left LogProcessor::OnEmit(std::unique_ptr<ReadWriteLogRecord> &&) only?
    • And also it's better to rename LogProcessor::MakeRecordable() to LogProcessor::MakeRecord()?
    • We can also implement ReadableLogRecord which inherit from ReadWriteLogRecord and make all writable functions private.
  • sdk::logs::LogRecord could be moved into ostream exporter, because it's only used in it and the name confuse with the specification.
  • OTLPFileExporter is mentioned at https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#otlp-file-exporter , but I didn't find more details about it. Some log system have rotation and achrive for file sinks.Or maybe it's better to provide a callback to let user forward the JSON or binary format data into their own log system.

I think these changes can be splited into several issues and implement them one by one.
Am I miss something or is there any suggestion?

[reyang]

First, I can rarely attend the meeting because the meeting time and timezone.(13:00 PT and 09:00PT are 04:00 AM and 00:00AM in my timezone). Sorry for that and the meeting time on wednesday has be moved up to 09:00PT(which is 00:00AM in my timezone) maybe I can attend this one sometimes.

I would vote for adjusting the meeting time to make sure everyone got covered.

I guess the 09:00PT meeting should be kept, the 13:00PT meeting can move to 16:00PT or 17:00PT?

[esigo]

that would be 01:00 - 02:00 for CET 😴

[reyang]

I think that's the key 😄
It is impossible to find one slot that fits everyone's time zone, however we can have rotating time slots, so one (9AM PT) would work for EMEA, and one (5PM PT) would work for APAC.

[esigo]

how about 8:00 PT? that would be 23:00 Beijing's time and 17:00 CET.

[owent]

how about 8:00 PT? that would be 23:00 Beijing's time and 17:00 CET.

That would be great for me.

[marcalff]

8:00 PT / 23:00 Beijing / 17:00 CET works for me as well.

[lalitb]

@owent Thanks for summarizing the changes required. Firstly, do we all agree this is going to be the breaking change, and to keep it simple, we won't support backward compatibility?

Here are the comments in general -

attributes should also be added into LoggerProvider::GetLogger(), should we merge the attributes from LoggerProvider::GetLogger and Logger::Log() before we exporting it to processor?

Yes, we should be merging both these attributes before emitting to processor.

include_trace_context should also be added into LoggerProvider::GetLogger(), but if user do not use trace::Scope , we can not get the current Span from RuntimeContext. Should we keep to use trace data from Logger::Log() or just use get Span from RuntimeContext ?

How about having Context as an optional/overload argument to Logger::EmitLogRecord() - use this if it has a valid span context, else try to get from runtimecontext. If none is present, don't emit trace information.

api::logs::LogRecord should be added into API to replace the logs::Recordable in SDK.

Recordable interface allows us to avoid storing span/log data internally before exporting it. I would prefer to keep it as it is - this may further mean we don't use LogRecord / ReadWriteLogRecord object at-all in code. We stick to the LogRecord data model. but don't use that as an object internally. What do you think?

Apart from that, there is also an event API which would need separate Logger::EmitEvent(name, {}} method.

Once we all agree with the scope, we can create separate issues accordingly.

[marcalff]

@owent Thanks for summarizing the changes required. Firstly, do we all agree this is going to be the breaking change, and to keep it simple, we won't support backward compatibility?

Ok with me. This is for logs, which are still experimental.

[owent]

How about having Context as an optional/overload argument to Logger::EmitLogRecord() - use this if it has a valid span context, else try to get from runtimecontext. If none is present, don't emit trace information.

Agree

Recordable interface allows us to avoid storing span/log data internally before exporting it. I would prefer to keep it as it is - this may further mean we don't use LogRecord / ReadWriteLogRecord object at-all in code. We stick to the LogRecord data model. but don't use that as an object internally. What do you think?

The specification says we should have a LogRecord in API. How about create a LogRecord in API, create ReadWriteLogRecord which derive from LogRecord, and make Recordable derive ReadWriteLogRecord ? Then we can still use Recordable in processors and exporters.

class LogRecord {
virtual ~LogRecord () = 0;
// Functions to set data
};

class ReadWriteLogRecord : public api::logs::LogRecord {
// Add functions to read data
};

class Recordable : public ReadWriteLogRecord ;

Apart from that, there is also an event API which would need separate Logger::EmitEvent(name, {}} method.

I prefer to keep only one prune function to emit logs. Or could we let EmitLogRecord() call EmitEvent() with empty event data in API?

[lalitb]

The specification says we should have a LogRecord in API. How about create a LogRecord in API, create ReadWriteLogRecord which derive from LogRecord, and make Recordable derive ReadWriteLogRecord ? Then we can still use Recordable in processors and exporters.

How will we create/get the LogRecord object inside the instrumented library if it is the pure virtual class? I mean the LogRecord is analogous to the Span object, and the Span object is created through Tracer, which also associates the right Recordable with it. Do we need something like this for Logs too ?

[owent]

How will we create/get the LogRecord object inside the instrumented library if it is the pure virtual class? I mean the LogRecord is analogous to the Span object, and the Span object is created through Tracer, which also associates the right Recordable with it. Do we need something like this for Logs too ?

Yes, I think it's a better way.

[ThomsonTan]

Agree with Reiley.

It is impossible to find one slot that fits everyone's time zone

For the log spec conformance review, could we have a one-off meeting at 8AM PT if it can work for most people? But I suggest we don't change the time for future series if it is less ideal for most audiences, like make it a obligated for @owent to join the meeting at 11PM in local time for every other week.

[owent]

Fine by me

[lalitb]

Based on these discussions, I have created the separate milestone for Logs API/SDK RC. The tasks are bit at microlevel (sorry about that), but I thought it would help us bring more clarity on the changes required.