Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] add time converter function #22811

Merged
merged 24 commits into from
Jun 22, 2023
Merged

Conversation

fchikwekwe
Copy link
Contributor

Description: Added a converter function that takes a time string and a format string and converts to a golang time.Time object.

Link to tracking Issue: #22007

Testing: Unit tests to check proper conversion from stringGetter to golang time.Time

Documentation:

@fchikwekwe fchikwekwe requested a review from a team May 26, 2023 14:59
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Please update the ottlfuncs/README with a new entry for this function.

pkg/ottl/ottlfuncs/func_to_time.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_to_time_test.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_to_time_test.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_to_time_test.go Outdated Show resolved Hide resolved
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please add a changelog entry by running chlog-new in the root directory of the repo. Also update pkg/ottl/go.mod with the new package dependency. Once you've updated the go.mod file run make gotidy in the root directory of the repo.

@github-actions github-actions bot added the processor/attributes Attributes processor label Jun 6, 2023
Co-authored-by: Tyler Helmuth <[email protected]>
TylerHelmuth
TylerHelmuth previously approved these changes Jun 7, 2023
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TylerHelmuth
Copy link
Member

While investigating these tests we found inconsistencies with timezones (it's always timezones). Comparing the time parsing to pkg/stanza/operator/helper TimeParser we found that using only the ctimefmt package was not enough as it did not handle timezones the same way TimeParser does.

Investigating further we found that this was the magic that made TimeParser work as it does:

result, err := time.ParseInLocation(t.Layout, str, t.location)
// Depending on the timezone database, we may get a pseudo-matching timezone
// This is apparent when the zone is not "UTC", but the offset is still 0
zone, offset := result.Zone()
if offset != 0 || zone == "UTC" {
return result, err
}
// Manually look up the location based on the zone
loc, locErr := time.LoadLocation(zone)
if locErr != nil {
// can't correct offset, just return what we have
return result, fmt.Errorf("failed to load location %s: %w", zone, locErr)
}
// Reparse the timestamp, with the location
resultLoc, locErr := time.ParseInLocation(t.Layout, str, loc)
if locErr != nil {
// can't correct offset, just return original result
return result, err
}
return resultLoc, locErr

and

if t.Location != "" {
// If "location" is specified, it must be in the local timezone database
loc, err := time.LoadLocation(t.Location)
if err != nil {
return fmt.Errorf("failed to load location %s: %w", t.Location, err)
}
t.location = loc
return nil
}
if strings.HasSuffix(t.Layout, "Z") {
// If a timestamp ends with 'Z', it should be interpretted at Zulu (UTC) time
t.location = time.UTC
return nil
}
t.location = time.Local
return nil

Handling the time in this way is necessary for a consistent time parsing strategy in the collector. Instead of duplicating these lines we should move the functionality out of stanza and into a shared place. See #23232 for implementation.

@TylerHelmuth TylerHelmuth self-requested a review June 13, 2023 16:57
@TylerHelmuth TylerHelmuth dismissed their stale review June 13, 2023 16:57

Solution changed, needs re-reviewed

pkg/ottl/ottlfuncs/func_time.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_time.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_time_test.go Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

@evan-bradley can you review this one as well? I'm too close to it after doing the refactor in timeutils to unblock it.

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Jun 22, 2023
@TylerHelmuth TylerHelmuth merged commit e5ccd1f into open-telemetry:main Jun 22, 2023
@github-actions github-actions bot added this to the next release milestone Jun 22, 2023
@ptimofee
Copy link

ptimofee commented Jul 6, 2023

Great stuff! Please update the ottlfuncs/README with a new entry for this function.

Not done

@TylerHelmuth
Copy link
Member

@ptimofee you are correct, I missed that. It is now being added via #23659

Caleb-Hurshman pushed a commit to observIQ/opentelemetry-collector-contrib that referenced this pull request Jul 6, 2023
Description: Added a converter function that takes a time string and a
format string and converts to a golang time.Time object.

Link to tracking Issue: open-telemetry#22007 

Testing: Unit tests to check proper conversion from `stringGetter` to
golang `time.Time`

Documentation:

---------

Co-authored-by: Tyler Helmuth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connector/count internal/filter pkg/ottl processor/attributes Attributes processor processor/filter Filter processor processor/routing Routing processor processor/span processor/tailsampling Tail sampling processor processor/transform Transform processor ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] OTTL Converter Function that is able to create a new time.Time type from a string.
4 participants