Skip to content
This repository was archived by the owner on May 21, 2024. It is now read-only.

Conversation

@datastream
Copy link
Contributor

not all servers' timezone setting is utc.

Copy link
Collaborator

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Edits to the parser should be made into the .rl file.

Anyway, it's not perfectly clear to me what you are trying to obtain in this case. Is theWithTimezone option what you want? What type of option do you want to implement? Could you please explain it better? Thanks :)

@datastream
Copy link
Contributor Author

In my servers , timezone setting is not utc.
example: In PST timezone, system print log like this

Nov 22 17:09:42 xxx kernel: [118479565.921459] EXT4-fs warning (device sda8): ext4_dx_add_entry:2006: Directory index full!

If I use default parse, it will convert to utc time 17:09:42. if I use WithTimezone,it will be converted to other. None of them match the real time.

@leodido
Copy link
Collaborator

leodido commented Apr 10, 2020

Which would be the "real time"?
Can you make a concrete example of the output timestamp you are looking for?

Also, your WithLocaleTimezone option returns the same value WithTimezone option (already existing) returns.

pst, _ := time.LoadLocation("PST")
i := []byte(`<13>Nov 22 17:09:42 xxx kernel: [118479565.921459] EXT4-fs warning (device sda8): ext4_dx_add_entry:2006: Directory index full!`)
p := NewParser(WithLocaleTimezone(pst))
m, _ := p.Parse(i)
// (*rfc3164.SyslogMessage)({
//  Base: (syslog.Base) {
//   Facility: (*uint8)(1),
//   Severity: (*uint8)(5),
//   Priority: (*uint8)(13),
//   Timestamp: (*time.Time)(0000-11-22 17:09:42 +0000 UTC),
//   Hostname: (*string)((len=3) "xxx"),
//   Appname: (*string)((len=6) "kernel"),
//   ProcID: (*string)(<nil>),
//   MsgID: (*string)(<nil>),
//   Message: (*string)((len=95) "[118479565.921459] EXT4-fs warning (device sda8): ext4_dx_add_entry:2006: Directory index full!")
//  }
// })

If you use NewParser(WithTimezone(pst)) you obtain the same output.

@datastream
Copy link
Contributor Author

package main

import (
        "fmt"
        "github.com/influxdata/go-syslog/v3/rfc3164"
        "time"
)

func main() {
        pst, _ := time.LoadLocation("America/New_York")
        i := []byte(`<13>Nov 22 17:09:42 xxx kernel: [118479565.921459] EXT4-fs warning (device sda8): ext4_dx_add_entry:2006: Directory index full!`)
       // I want Nov 22 17:09:42 in LMT not UTC
        p := rfc3164.NewParser(rfc3164.WithTimezone(pst))
        m, _ := p.Parse(i)
        fmt.Println(m)
        p = rfc3164.NewParser(rfc3164.WithLocaleTimezone(pst))
        m, _ = p.Parse(i)
        fmt.Println(m)
        p = rfc3164.NewParser()
        m, _ = p.Parse(i)
        fmt.Println(m)
}

return

&{{0xc00001408b 0xc00001408c 0xc00001408a 0000-11-22 12:13:40 -0456 LMT 0xc000050200 0xc000050210 <nil> <nil> 0xc000050230}}
&{{0xc0000140d7 0xc0000140d8 0xc0000140d6 0000-11-22 17:09:42 -0456 LMT 0xc000050260 0xc000050270 <nil> <nil> 0xc000050290}}
&{{0xc0000140f7 0xc0000140f8 0xc0000140f6 0000-11-22 17:09:42 +0000 UTC 0xc0000502c0 0xc0000502d0 <nil> <nil> 0xc0000502f0}}

WithTimezone will convert UTC time "0000-11-22 17:09:42 +0000 UTC" to "0000-11-22 12:13:40 -0456 LMT".

@leodido
Copy link
Collaborator

leodido commented Apr 15, 2020

Ok @datastream, I updated the Ragel definition, generated the parser from it, updated the docs, and inserted an example for the new WithLocaleTimezone option you proposed.

Can you please double-check now it's behavior is as intended? Thanks!

@leodido leodido self-requested a review April 16, 2020 14:26
Copy link
Contributor Author

@datastream datastream left a comment

Choose a reason for hiding this comment

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

Please update doc

}

func Example_withlocaletimezone() {
pst, _ := time.LoadLocation("PST")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please use time.LoadLocation("America/New_York") instead. time.LoadLocation("PST") will not work.

// Facility: (*uint8)(1),
// Severity: (*uint8)(5),
// Priority: (*uint8)(13),
// Timestamp: (*time.Time)(0000-11-22 17:09:42 +0000 UTC),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use please use time.LoadLocation("America/New_York"), it will return Timestamp: (*time.Time)(0000-11-22 17:09:42 +0000 LMT)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I obtain 0000-11-22 17:09:42 -0456 LMT not 0000-11-22 17:09:42 +0000 LMT

Copy link
Collaborator

Choose a reason for hiding this comment

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

As you can see in the sent commit

@datastream
Copy link
Contributor Author

It looks good to me.

@leodido leodido merged commit 5da5efe into influxdata:develop Apr 17, 2020
} else {
if m.timezone != nil {
t, _ = time.ParseInLocation(time.Stamp, string(m.text()), m.timezone)
}
Copy link

Choose a reason for hiding this comment

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

Would you be open to a refactor here so that the time isn't parsed twice here where the first result ends up being ignored?

I think it can be achieved by always using ParseInLocation but then with UTC if no custom option is given?

Copy link

Choose a reason for hiding this comment

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

Additionally, there's no release yet with this functionality included, would it be possible to make a new release as well (maybe even after adding this optimization?).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd love to look at a PR with such optimization @dbussink

Copy link
Contributor Author

Choose a reason for hiding this comment

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

diff --git a/rfc3164/machine.go.rl b/rfc3164/machine.go.rl
index 4d13817..6629696 100644
--- a/rfc3164/machine.go.rl
+++ b/rfc3164/machine.go.rl
@@ -38,20 +38,37 @@ action set_prival {
 }

 action set_timestamp {
-       if t, e := time.Parse(time.Stamp, string(m.text())); e != nil {
-               m.err = fmt.Errorf("%s [col %d]", e, m.p)
-               fhold;
-               fgoto fail;
-       } else {
-               if m.timezone != nil {
-                       t, _ = time.ParseInLocation(time.Stamp, string(m.text()), m.timezone)
-               }
-               output.timestamp = t.AddDate(m.yyyy, 0, 0)
-               if m.loc != nil {
-                       output.timestamp = output.timestamp.In(m.loc)
-               }
-               output.timestampSet = true
-       }
+       if m.timezone != nil {
+        if t, e := time.ParseInLocation(time.Stamp, string(m.text()), m.timezone); e != nil {
+            m.err = fmt.Errorf("%s [col %d]", e, m.p)
+            (m.p)--
+
+            {
+                goto st373
+            }
+        } else {
+            output.timestamp = t.AddDate(m.yyyy, 0, 0)
+            if m.loc != nil {
+                output.timestamp = output.timestamp.In(m.loc)
+            }
+            output.timestampSet = true
+        }
+    } else {
+        if t, e := time.Parse(time.Stamp, string(m.text())); e != nil {
+            m.err = fmt.Errorf("%s [col %d]", e, m.p)
+            (m.p)--
+
+            {
+                goto st373
+            }
+        } else {
+            output.timestamp = t.AddDate(m.yyyy, 0, 0)
+            if m.loc != nil {
+                output.timestamp = output.timestamp.In(m.loc)
+            }
+            output.timestampSet = true
+        }
+    }
 }

 action set_rfc3339 {

How about this one?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants