chore: Added comments to the duplicate event.name code in EventLogger

[jerrytfleung]

Fixed duplicated code found in the emit function.

UPDATES: It is not duplicate code. It turns out it is intended to get the first placeholder for event.name in attributes so it won't be dropped due to the OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT limitation scenario.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.43%. Comparing base (89ca91d) to head (48d3c24).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1768      +/-   ##
============================================
+ Coverage     68.38%   68.43%   +0.04%     
  Complexity     2970     2970              
============================================
  Files           448      448              
  Lines          8712     8714       +2     
============================================
+ Hits           5958     5963       +5     
+ Misses         2754     2751       -3     

Flag	Coverage Δ
8.1	68.08% <ø> (-0.02%)	⬇️
8.2	68.36% <ø> (+0.07%)	⬆️
8.3	68.31% <ø> (-0.01%)	⬇️
8.4	68.31% <ø> (+0.03%)	⬆️
8.5	68.28% <ø> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/SDK/Logs/EventLogger.php	100.00% <ø> (ø)

... and 4 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89ca91d...48d3c24. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Nevay]

This was done on purpose to ensure that we don't drop the event.name attribute.

[jerrytfleung]

This was done on purpose to ensure that we don't drop the event.name attribute.

Previously, event.name attribute could be replaced if $attributes contains another event.name in setAttributes call

$logRecord->setAttributes($attributes);

While the code set the event.name again after the setAttributes call to ensure we don't lose that attribute, so I think we don't need the first event.name set attributes calls. (The later always wins)

[brettmc]

This was done on purpose to ensure that we don't drop the event.name attribute.

I have a memory of this, too. There is a unit test for it, and if that is passing then I think this change is OK now.

[Nevay]

The linked test verifies that the $name parameter takes precedence over an event.name attribute provided in $attributes (= the second $logRecord->setAttribute('event.name', $name)).

The first $logRecord->setAttribute('event.name', $name) ensures that event.name is the first attribute entry -> is not dropped by attribute limits (unless 0).

#[BackupGlobals(true)]
public function test_event_name_is_not_dropped_first(): void
{
	$_SERVER['OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT'] = 1;
	$eventLoggerProvider = new EventLoggerProvider((new LoggerProviderBuilder())
		->addLogRecordProcessor(new SimpleLogRecordProcessor($exporter = new InMemoryExporter()))
		->build());
	$eventLogger = $eventLoggerProvider->getEventLogger('test');

	Logging::disable();
	try {
		$eventLogger->emit('my.event', attributes: ['other.attribute' => 'not.my.event']);
	} finally {
		Logging::reset();
	}
	$eventLoggerProvider->forceFlush();

	/** @var list<ReadableLogRecord> $logs */
	$logs = $exporter->getStorage()->getArrayCopy();
	$this->assertCount(1, $logs);
	$this->assertArrayHasKey('event.name', $logs[0]->getAttributes()->toArray());
}

[jerrytfleung]

Closing this PR as the function call is intended to set the first entry of the attributes so that it won't get dropped due to OTEL_LOGRECORD_ATTRIBUTE_COUNT_LIMIT scenario

[ChrisLightfootWild]

Does this warrant adding a comment to the existing code to explain the scenario that is being covered by this?

[brettmc]

@jerrytfleung thanks for the update. Could you also include the test Nevay provided above in this PR?