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] Adapt time parsing error messages to include the ctime directives #38425

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Mar 6, 2025

Description

This PR adapts the Time function to check for time.ParseError errors. If such an error is received by the function, it wraps it into a custom error type in order to include the provided ctime layout string instead of the go-native layout string.

Link to tracking issue

Fixes #35176

Testing

Adapted the unit tests of the Time() function

bacherfl added 2 commits March 6, 2025 08:21
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
…rmat

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
bacherfl added 3 commits March 7, 2025 07:53
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
@bacherfl bacherfl marked this pull request as ready for review March 10, 2025 09:18
@bacherfl bacherfl requested a review from a team as a code owner March 10, 2025 09:18
return res
}

var nativeToCtimeSubstitutes = map[string][]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking about ways of not having this logic/substitutes replicated here, and I think we could maybe achieve that by adding a new function into the ctimefmt package that takes a ctime-like format and returns a map from the Go's native elements to theirs c-time substitutes, something like:

func GetNativeSubstitutes(format string) map[string]string {
	nativeDirectives := map[string]string{}
	directives := ctimeRegexp.FindAllString(format, -1)
	for _, directive := range directives {
		if val, ok := ctimeSubstitutes[directive]; ok {
			nativeDirectives[val] = directive
		}
	}
	return nativeDirectives
}

With that method, we could then create the lookup map outside of the returned function (once), and just do the lookup whenever the parsing error happens. It wouldn't be necessary to handle duplicate directives as well, as the function would only return the used native elements. WDYT?

func toCTimeError(parseError time.ParseError, format string) error {
res := &ctimeError{timeErr: parseError}
// set the layout to the originally provided ctime format
res.timeErr.Layout = format
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be safer to not modify the time.ParseError fields. We could maybe have a custom wrapper instead, that mimics the time.ParseError.Error() behavior but using the directives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Improve Time converter error messages
3 participants