Logs API - initialize LogRecordData.Timestamp with MinValue#7045
Logs API - initialize LogRecordData.Timestamp with MinValue#7045Kielek wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
github-actions
bot
added
pkg:OpenTelemetry.Api
github-actions
bot
added
the
pkg:OpenTelemetry
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7045 +/- ##
==========================================
+ Coverage 88.77% 88.82% +0.05%
==========================================
Files 270 270
Lines 12929 12935 +6
==========================================
+ Hits 11478 11490 +12
+ Misses 1451 1445 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@juliuskoval, could you please review? |
| if (logTimestamp != DateTime.MinValue) | ||
| { | ||
| // Timestamp was explicitly set: use it for both fields. | ||
| timeUnixNano = (ulong)logTimestamp.ToUnixTimeNanoseconds(); | ||
| observedTimeUnixNano = timeUnixNano; | ||
| } | ||
| else | ||
| { | ||
| // Timestamp not set -> time_unix_nano = 0 ("unknown or missing" per OTLP spec). | ||
| // observed_time_unix_nano MUST still be populated (proto spec requirement). | ||
| timeUnixNano = 0; | ||
| observedTimeUnixNano = (ulong)DateTime.UtcNow.ToUnixTimeNanoseconds(); | ||
| } |
There was a problem hiding this comment.
Have you considered changing ToUnixTimeNanoseconds (and other extension methods) to return 0 when datetime is min?
There was a problem hiding this comment.
Yes, but we should handle timestamp and observed timespan differently, so I would keep as is.
| { | ||
| // Timestamp was explicitly set: use it for both fields. | ||
| timeUnixNano = (ulong)logTimestamp.ToUnixTimeNanoseconds(); | ||
| observedTimeUnixNano = timeUnixNano; |
There was a problem hiding this comment.
Why not always setting observedTimeUnixNano = (ulong)DateTime.UtcNow.ToUnixTimeNanoseconds();?
This field SHOULD be set once the event is observed by OpenTelemetry.
I am not sure what would be the value of setting the same time for ObservedTimestamp and Timestamp.
Here is what we do in OTel Go: https://github.com/open-telemetry/opentelemetry-go/blob/3157d0a7d9d22c3343838da6b1a691557676072f/sdk/log/logger.go#L116-L119
There was a problem hiding this comment.
Setting ObservedTimestamp is addressed in #6979; this PR only addresses nullability of Timestamp if I understand it correctly. Changes from the other PR would allow users to set ObservedTimestamp in the API/SDK.
There was a problem hiding this comment.
Julius is right, it is intermittent state and should be changed in the short cadence in #6973.
I hope that both will be merged together. For now, I am keeping existing behavior as much as possible.
You didn't add changelogs for |
|
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
bot
added
the
Stale
Kielek
removed
the
Stale
3 participants
Handles discussions from #6979 related to nullability.
Changes
Logs API - initialize LogRecordData.Timestamp with MinValue
MinValue is equivalent of 0/null value from OTel Spec.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)