[DSET-4071] [DSET-4007] Fix timestamp handling for LogRecord objects without a timestamp #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
observed timetamp in case LogRecord doesn't contain timestamp. Even though ObservedTimestamp should always be present, we fall back to current time in case it's not. In addition to that, remove duplicate and redundant "timestamp" attribute from the AddEvents event.
zdaratom-s1
approved these changes
Jun 9, 2023
| @@ -229,6 +228,65 @@ func TestBuildEventFromLog(t *testing.T) { | |||
| assert.Equal(t, expected, was) | |||
| } | |||
|
|
|||
| func TestBuildEventFromLogEventWithoutTimestampWithObservedTimestampUseObservedTimestamp(t *testing.T) { | |||
There was a problem hiding this comment.
Ideally we structure our tests in GIVEN, WHEN and THEN section to increase readability
There was a problem hiding this comment.
I'm fine with it as long as it's consistent across the board (and also in line with go conventions / best practices) :)
…try-collector-contrib into event_no_timestamp_fix_use_occurence_time
|
Build failures seem to be related to other exporters and not ours, so I will ignore them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request updates the code to make sure addEvents Event object always has a valid timestamp set.
In addition to that, I removed redundant and unnecessary
timestampfield from the event attributes. This field is already set as part of the mainevent.tsfield and storing it also as part of the event attributes is causing various issues on the server side.Background, Context
While working on a benchmarking task, I noticed that sometimes
LogRecordobjects have timestamp set to the beginning of the unix epoch (January 1970).After digging in, I saw this could happen in various scenarios since it's a default value in case timestamp is not set / available - e.g. if we use
filelogreceiver and timestamp is not explicitly parsed from the log line (if available on the line at all) as part of the filelog parser operator.This is problematic and it means those events won't get ingested / handled correctly on DataSet side since they have timestamp set to a very long time ago.
Proposed Solution / Fix
To solve this problem, I set event timestamp to the
ObservedTimestampin case timestamp is not set / available on theLogRecordobject.ObservedTimestampholds the timestamp when the collector observed the event (e.g. when the line was read from file or similar). It should always be set, but to be extra safe / as a fallback, I set event timestamp to current time in case neitherLogRecord.TimestampnorLogRecord.ObservedTimestampis available.