Logs does not enforce limits

[cijothomas]

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#logrecord-limits

[Shunpoco]

Hi @cijothomas, can I take this ticket?

In my understanding, what we should do are:

1. Define struct Config for Logs and set it into LoggerProviderInner. The Config keeps attribute_count_limit and attribute_value_length_limit fields, and they are initialised through environmental variables.
2. Set those two limits into LogRecord. LogRecord will use them in add_attribute(s) methods to enforce the limitations.
3. LoggerProviderBuilder needs with~ methods like with_attribute_value_length_limit() to set those values.

[lalitb]

Thanks for volunteering this.

Set those two limits into LogRecord. LogRecord will use them in add_attribute(s) methods to enforce the limitations.

Please see if it is possible to implement it without increasing the record size.

[Shunpoco]

@lalitb thanks, could you assign me to this ticket?

without increasing the record size

Let me clarify, do you concern a size of each LogRecord instance? or is it about actual log which a processor composes from those instances?
In my current design, each LogRecord instance will have two additional u32 fields but those fields doesn't show on actual logs.

[lalitb]

In my current design, each LogRecord instance will have two additional u32 fields but those fields doesn't show on actual logs.

I was thinking if we can avoid that, as these two additional fields are configured at provider level and so will have same value across all the record instances. But don't see an easy way to do it for now, let's discuss this over the PR.

[cijothomas]

I'd suggest to have more detailed design/discussion before attempting to implement this. It is unclear what is the need for enforcing the limits - if this is to comply with backends that has limits, then this maybe better off be done at the OTLP Exporter level.
If the goal is to protect the SDK from taking too much memory, then this maybe suited for BatchProcessor as that is the component who stores LogRecord on heap. (LogRecord is on stack only otherwise.)

#1283 some prior discussions. Not everything is applicable here, Log API is redesigned to not to store anything, but delegates everything to the SDK.

(Not opposed to the change, just trying to ensure we have the end goal clear and won't affect perf)

[cijothomas]

do you concern a size of each LogRecord instance

Yes, It is stack allocated and clone/copied to Heap on BatchProcessor, so any increase in size will likely have some perf impact.

[Shunpoco]

Thanks both!

Sure, I will consider the design and then let's discuss (probably here or on weekly meeting?)

[Shunpoco]

Hi @cijothomas and @lalitb

I wrote a design document here: https://docs.google.com/document/d/1S-N9_cClLuOjthcpXATKCeHQXsTz5UWihG8UpV2ttGw/edit?usp=sharing. Please give me a feedback, or if you prefer another way such as writing it down on this issue or chatting in a meeting, please let me know.