fix: update total_recorded_attributes when LogRecord#attributes= is called

[ltickett]

Closes #2194

The attr_accessor-generated attributes= setter reassigned the attributes hash but left @total_recorded_attributes at its initialization value, so to_log_record_data reported a stale count after mutation (e.g. in a custom LogRecordProcessor#on_emit).

Replace the generated setter with a custom attributes= that recalculates @total_recorded_attributes and reapplies the configured attribute limits, and reuse it from the constructor.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: ltickett / name: Lee Tickett (01c94d6)

[thompson-tomo]

Looking at this change, it improves the situation however it doesn't appear to address adding/removing attributes which is supported in the spec.

Chatting with copilot, it came up with:

Added a trackedAttributes class

class TrackedAttributes
  attr_reader :dropped_count

  def initialize(initial = {})
    @attributes = initial.dup
    @dropped_count = 0
  end

  def delete(key)
    existed = @attributes.key?(key)
    result = @attributes.delete(key)
    @dropped_count += 1 if existed
    result
  end

  def delete_if(&block)
    @attributes.delete_if do |k, v|
      should_drop = block.call(k, v)
      @dropped_count += 1 if should_drop
      should_drop
    end
  end

  # Forward everything else to the underlying hash
  def method_missing(name, *args, &block)
    @attributes.public_send(name, *args, &block)
  end

  def respond_to_missing?(name, include_private = false)
    @attributes.respond_to?(name, include_private)
  end
end

And we change our logrecord to be

class LogRecordData
  attr_accessor :attributes

  def initialize(...)
    @attributes = TrackedAttributes.new({})
  end

  def dropped_attributes_count
    @attributes.dropped_count
  end
end

That solution can be scaled to support adding/updating of attributes including having the limits enforced on those additional operations. We can also reuse it for spans etc

[kaylareopelle]

Looking at this change, it improves the situation however it doesn't appear to address adding/removing attributes which is supported in the spec...

@thompson-tomo, thanks for reviewing this with missing features in mind! This PR does a great job resolving the total_recorded_attributes bug. Adding/removing attributes feels like a new feature to me that I'd rather see addressed in a separate PR.

[kaylareopelle]

Ah, interesting! Why go with self.attributes here? Just want to make sure I'm following the flow.

[ltickett]

Thanks @thompson-tomo - I agree with @kaylareopelle - let's look at this in a follow-up and keep this PR scope small.

Ah, interesting! Why go with self.attributes here? Just want to make sure I'm following the flow.

@kaylareopelle calling self.attributes = attributes in the constructor (log_record.rb:92) routes through the new setter (log_record.rb:102) so that @total_recorded_attributes calculation and trim_attributes happen in exactly one place. Without it you'd duplicate the count + trim logic in both the constructor and the setter, which is what caused issue #2194 in the first place (setter diverged from constructor). So reusing the setter seems like the logical DRY choice.

Wdyt?

[thompson-tomo]

I have no problem with doing it in small stages however if we do so, we should avoid saying that this pr fixes the issue when it doesn't and ensure that the bug is kept open.

In fact on thinking about it, this solution might lead to scenarios which were previously correct now being wrong. For instance create a log record with 5 attributes, then update the attributes to contain 4 using set. With using total when attributes is set you would no longer know about the dropped attribute.

[kaylareopelle]

I have no problem with doing it in small stages however if we do so, we should avoid saying that this pr fixes the issue when it doesn't and ensure that the bug is kept open.

In fact on thinking about it, this solution might lead to scenarios which were previously correct now being wrong. For instance create a log record with 5 attributes, then update the attributes to contain 4 using set. With using total when attributes is set you would no longer know about the dropped attribute.

@thompson-tomo, I'm not sure I'm following. Could you provide a code snippet that creates this situation or write a test that fails with the current code?

[thompson-tomo]

@kaylareopelle Here is the test

it 'updates total_recorded_attributes when reassigned' do
        log_record = Logs::LogRecord.new(attributes: { 'key1' => 'value1', 'key2' => 'value2', 'key3' => 'value3' })
        assert_equal(3, log_record.instance_variable_get(:@total_recorded_attributes))

        log_record.attributes = { 'key1' => 'value1' }

        assert_equal(3, log_record.instance_variable_get(:@total_recorded_attributes))
        assert_equal(3, log_record.to_log_record_data.total_recorded_attributes)
        assert_equal(2, log_record.to_log_record_data.total_dropped_attributes)
      end

In effect it is the opposite to the test which now passes

[kaylareopelle]

@thompson-tomo - Thank you, I see what you're saying. I think in this case though, shouldn't total_recorded_attributes be 4, because four attributes have been seen, even though one remains? The new hash is effectively overwriting the old hash, so even though it's the same key/value pair content, I think we should count it as a new attribute.

This would return the expected 3 for dropped_attributes_count in the OTLP logs exporter:

opentelemetry-ruby/exporter/otlp-logs/lib/opentelemetry/exporter/otlp/logs/logs_exporter.rb

Line 312 in 85b5248

dropped_attributes_count: log_record_data.total_recorded_attributes - log_record_data.attributes&.size.to_i,

Does this logic make sense to you, @ltickett? I'll add a code comment with a way we could adjust things if we think the value should resolve to four in @thompson-tomo's previous comment.

[kaylareopelle]

I just realized we've strayed from the spec. @total_recorded_attributes should only change if an attribute is dropped due to log limits, otherwise, it remains identical to the attributes size of the payload that gets sent up. That would likely move this logic to trim_attributes.

Here's the definition for dropped_attributes_count in the Traces proto (the definition isn't in the Logs proto).

  // The number of attributes that were discarded. Attributes
  // can be discarded because their keys are too long or because there are too many
  // attributes. If this value is 0, then no attributes were dropped.

https://github.com/open-telemetry/opentelemetry-proto/blob/46f1e4ce1d903009f3e38e3052ce8eab5c4e4d0c/opentelemetry/proto/trace/v1/trace.proto#L215-L218

Changing attributes during the life of a Log Record isn't enough to make that attribute "dropped" and change the total_attributes_count.

[thompson-tomo]

I agree in my scenario the total could be considered 4 but it is under defined.

Looking at https://opentelemetry.io/docs/specs/otel/logs/sdk/#readwritelogrecord is it even supported to be replacing the entire collection?

Would it make more sense to track the dropped count directly that we could track all removals.

I have asked questions in the spec channel as area feels under defined.

[ltickett]

@thompson-tomo @kaylareopelle did we land on a decision regarding the way forward here please?

[kaylareopelle]

Hi @ltickett, apologies for the delayed response. We didn't get any feedback in Slack. I'm going to bring this to the SIG meeting on Tuesday to try to get a consensus and we'll have more for you after that.

[kaylareopelle]

During the SIG meeting, it was suggested that I look at other implementations to see if there's a consistent approach for handling this value.

After reviewing Python, JS, and Java, I think it's safe to say that dropped_attributes_count relates to attributes dropped because they exceeded the total attribute limit (OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT or OTEL_ATTRIBUTE_COUNT_LIMIT).

• Python test
• JS test
• Java test - In Java, the AttributesMap is used for both logs and traces, so the traces prototype definition seems to apply to both cases.

@thompson-tomo, what do you think about the spec interpretation given these examples?

cc @mwear @xuan-cao-swi

[thompson-tomo]

I have no issue with dropped being scoped to counting those dropped due to limits. With that being said, i still don't see this change as resolving the issue and not even progressing it.

If we were to instead simply track how many attributes have been dropped due to limits, we can then calculate the total.

Some test cases i can think of would be:

• create log record with limit + 2 attributes, add 1 attribute, remove 1 attribute, add 2 attributes. The dropped count should ideally be 4 but 3 is explainable.

If we only use the size prior to last limiting we would end up with 1.