Add ObservedTimestamp to LogRecordData

[juliuskoval]

Towards #4433

Changes

Adds ObservedTimestamp to LogRecordData and LogRecord.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[juliuskoval]

On a somewhat related note, it seems to me that the spec suggests that LogRecordData.Timestamp should be nullable. LogRecord.Timestamp is always set in OpenTelemetryLogger so it shouldn't be a breaking change, but it would be changing the stable public API.

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.81%. Comparing base (0f74489) to head (7f596a8).
⚠️ Report is 66 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/OpenTelemetry/Logs/LogRecord.cs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6979      +/-   ##
==========================================
- Coverage   88.87%   88.81%   -0.07%     
==========================================
  Files         263      263              
  Lines       12424    12434      +10     
==========================================
+ Hits        11042    11043       +1     
- Misses       1382     1391       +9     

Flag	Coverage Δ
unittests-Project-Experimental	88.62% <90.90%> (-0.14%)	⬇️
unittests-Project-Stable	88.53% <90.90%> (-0.31%)	⬇️

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

Files with missing lines	Coverage Δ
src/OpenTelemetry.Api/Logs/LogRecordData.cs	100.00% <100.00%> (ø)
...metry.Exporter.Console/ConsoleLogRecordExporter.cs	97.10% <100.00%> (+0.04%)	⬆️
...ementation/Serializer/ProtobufOtlpLogSerializer.cs	99.24% <100.00%> (+0.01%)	⬆️
.../OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs	87.00% <100.00%> (+0.13%)	⬆️
src/OpenTelemetry/Logs/LogRecord.cs	70.58% <66.66%> (-0.08%)	⬇️

... and 2 files with indirect coverage changes

[rajkumar-rangaraj]

Left a suggestion to update OTLP Exporter changelog.

[Kielek]

On a somewhat related note, it seems to me that the spec suggests that LogRecordData.Timestamp and LogRecord.Timestamp should be nullable. LogRecord.Timestamp is always set in OpenTelemetryLogger so it shouldn't be a breaking change, but it would be changing the stable public API.

I think that we should document this spec deviation in the document (can be separate PR). What is more it will be great to create some issue to fix it. There is label required major bump.

[juliuskoval]

On a somewhat related note, it seems to me that the spec suggests that LogRecordData.Timestamp and LogRecord.Timestamp should be nullable. LogRecord.Timestamp is always set in OpenTelemetryLogger so it shouldn't be a breaking change, but it would be changing the stable public API.

I think that we should document this spec deviation in the document (can be separate PR). What is more it will be great to create some issue to fix it. There is label required major bump.

I created this issue to track all the deviations from the spec. #6992

[Kielek]

Sorry for delays with reviews. Finally, I was able to finish other OTel Auto task.

If we go with non-nullable approach, it should be fine with some small adjustments.

I have checked also other possibilities (the code is still dirty and probably needs to be split into at least 2 PRs, but it shows potential solution without breaking changes.
Description https://github.com/Kielek/opentelemetry-dotnet/blob/3ee221fe48f17ca399b3dd4d59d7cca9cf8b96de/docs/design/make-timestamp-nullable.md

and the PR itself: Kielek@3ee221f

[Kielek]

Similar change should be done for ConsoleExporter.
Together with Timestamp, we should log also Observed timestamp.

Please add also CHANGELOG entry.

[Kielek]

Consider, I think that we should avoid any drifts between these values by default.

Suggested change

	internal DateTime ObservedTimestampBacking = DateTime.UtcNow;
	internal DateTime ObservedTimestampBacking = TimestampBacking;

[juliuskoval]

@Kielek Sorry about taking so long as well. Regarding your proposed solutions, couldn't we just leave LogRecord.Timestamp as it is right now given that all options would lead to deviations from the spec and document the difference somewhere?

[juliuskoval]

Hi @Kielek, do you have any thoughts on my previous comment?

[Kielek]

Yes, I have one more discussion about this last days. We could change a bit behavior in non-public class and treat DateTime.MinValue/default as not set (for both values) + check it during exporting.

As I know Go-lang is implemented in this way. Hidden null, to avoid extra memory footprint by Nullable version.

I will play a bit with thin Tuesday+ next week.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.