Skip to content
This repository has been archived by the owner on May 25, 2022. It is now read-only.

Improve no timezone db error #289

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Improve no timezone db error #289

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c87870a
Wrap error from time.LoadLocation to include reason
BinaryFissionGames Oct 12, 2021
25af759
time zone -> timezone
BinaryFissionGames Oct 12, 2021
80c1437
Remove timezone database suggestion
BinaryFissionGames Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion operator/builtin/parser/syslog/syslog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (c SyslogParserConfig) Build(context operator.BuildContext) ([]operator.Ope

location, err := time.LoadLocation(c.Location)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to load location %s (is the timezone database installed?): %w", c.Location, err)
Copy link
Member

Choose a reason for hiding this comment

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

I can appreciate what you're getting at here, but an error message that asks a question is unusual. More importantly, I suspect this will be a red herring in the majority of cases, since the most common cause of this error will be users specifying an invalid location.

Copy link
Member

Choose a reason for hiding this comment

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

For context, on Windows, the timezone database is generally not present. While on Linux it is common to have the tzdata package installed by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was kinda iffy on about putting that in there, too. I'll take the question out. Knowing that it failed to load the location should probably be enough to debug.

}

syslogParser := &SyslogParser{
Expand Down
11 changes: 11 additions & 0 deletions operator/builtin/parser/syslog/syslog_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"time"

"github.com/stretchr/testify/require"
"go.uber.org/zap"
"gopkg.in/yaml.v2"

"github.com/open-telemetry/opentelemetry-log-collection/entry"
Expand Down Expand Up @@ -101,3 +102,13 @@ parse_to: $.to`
require.Equal(t, expect, &actual)
})
}

func TestSyslogParserInvalidLocation(t *testing.T) {
config := NewSyslogParserConfig("test")
config.Location = "not_a_location"
config.Protocol = RFC3164

_, err := config.Build(operator.NewBuildContext(zap.NewNop().Sugar()))
require.Error(t, err)
require.Contains(t, err.Error(), "failed to load location "+config.Location)
}
4 changes: 2 additions & 2 deletions operator/helper/time.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (t *TimeParser) setLocation() error {
// If "location" is specified, it must be in the local timezone database
loc, err := time.LoadLocation(t.Location)
if err != nil {
return err
return fmt.Errorf("failed to load location %s (is the timezone database installed?): %w", t.Location, err)
}
t.location = loc
return nil
Expand Down Expand Up @@ -198,7 +198,7 @@ func (t *TimeParser) parseGotime(value interface{}) (time.Time, error) {
loc, locErr := time.LoadLocation(zone)
if locErr != nil {
// can't correct offset, just return what we have
return result, err
return result, fmt.Errorf("failed to load location %s (is the timezone database installed?): %w", zone, locErr)
}

// Reparse the timestamp, with the location
Expand Down
21 changes: 21 additions & 0 deletions operator/helper/time_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,3 +736,24 @@ func defaultTimeCfg() *TimeParser {
newCfg := NewTimeParser()
return &newCfg
}

func TestSetInvalidLocation(t *testing.T) {
tp := NewTimeParser()
tp.Location = "not_a_location"
err := tp.setLocation()
require.Error(t, err)
require.Contains(t, err.Error(), "failed to load location "+"not_a_location")
}

func TestParseGoTimeBadLocation(t *testing.T) {
tp := NewTimeParser()
tp.Location = "America/New_York"

err := tp.setLocation()
require.NoError(t, err)

tp.Layout = time.RFC822
_, err = tp.parseGotime("02 Jan 06 15:04 BST")
require.Error(t, err)
require.Contains(t, err.Error(), "failed to load location BST")
}