Initial implementation #3

Merged
alexvanin merged 1 commit from AlekseySVTN/zapjournald:feature/0-initial_implementation into master 2023-04-10 14:24:36 +00:00
Member

Signed-off-by: Aleksey Savaitan zxc_off@mail.ru

This is the first PR.
Commits with good "godoc" comments coming soon. May be already in this PR.

Signed-off-by: Aleksey Savaitan <zxc_off@mail.ru> **This is the first PR. Commits with good "godoc" comments coming soon. May be already in this PR.**
AlekseySVTN requested review from alexvanin 2023-04-04 13:53:51 +00:00
AlekseySVTN requested review from vdomnich-yadro 2023-04-04 13:54:11 +00:00
AlekseySVTN requested review from fyrchik 2023-04-04 14:02:23 +00:00
ironbee requested review from ironbee 2023-04-04 14:05:16 +00:00
fyrchik requested review from storage-core-developers 2023-04-04 14:09:11 +00:00
fyrchik requested review from storage-core-committers 2023-04-04 14:09:11 +00:00
alexvanin reviewed 2023-04-04 14:43:34 +00:00
Readme.md Outdated
@ -0,0 +1,148 @@
# Zap Core for systemd Journal
Owner

Usually we name it README.md with uppercase in TrueCloudLab repos.

Usually we name it `README.md` with uppercase in TrueCloudLab repos.
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
Readme.md Outdated
@ -0,0 +8,4 @@
```go
package main
import (
Owner

Let's add an issue to simplify this example. I think we can remove parsing and setting of log level, date encoding, etc.

Let's add an issue to simplify this example. I think we can remove parsing and setting of log level, date encoding, etc.
Author
Member

Issue was created: #4 +

Issue was created: https://git.frostfs.info/TrueCloudLab/zapjournald/issues/4 \+
AlekseySVTN marked this conversation as resolved
Readme.md Outdated
@ -0,0 +75,4 @@
sudo journalctl -f
апр 04 12:23:38 aleksey-savaitan ___go_build_git_frostfs_info_TrueCloudLab_zapjournald_main[143813]: {"level":"info","ts":"2023-04-04T12:23:38.784+0300","msg":"Simple log raw 1","SYSLOG_FACILITY":3,"SYSLOG_IDENTIFIER":"___go_build_git_frostfs_info_TrueCloudLab_zapjournald_main","SYSLOG_PID":143813}
апр 04 12:23:38 aleksey-savaitan ___go_build_git_frostfs_info_TrueCloudLab_zapjournald_main[143813]: {"level":"error","ts":"2023-04-04T12:23:38.784+0300","msg":"Simple log raw 2","SYSLOG_FACILITY":3,"SYSLOG_IDENTIFIER":"___go_build_git_frostfs_info_TrueCloudLab_zapjournald_main","SYSLOG_PID":143813,"JUST":1}
```
Owner

Let's update results with setting 'LC_LANG=C' to avoid cyrillic dates and hide personal information like the name.

Let's update results with setting 'LC_LANG=C' to avoid cyrillic dates and hide personal information like the name.
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
Readme.md Outdated
@ -0,0 +85,4 @@
#### Or you can read full-structured view like this:
```
sudo journalctl SYSLOG_PID=76385 --output=json-pretty
{
Owner

One log record is enough to demonstrate, I think.

One log record is enough to demonstrate, I think.
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
@ -0,0 +1,153 @@
package zapjournald
Owner

Let's use one-word names for files. I think zapjournald.go and fields.go looks better, but it is a matter of taste.

Let's use one-word names for files. I think `zapjournald.go` and `fields.go` looks better, but it is a matter of taste.
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
@ -0,0 +8,4 @@
"golang.org/x/exp/maps"
)
// Core
Owner

It will be nice for all public structures and functions to be documented. Here we can mention, that Core is a structure that implements zap.Core interface for logging into journald.

You can use comments from interface definition here: 845ca51d5b/zapcore/core.go (L23-L44)

It will be nice for all public structures and functions to be documented. Here we can mention, that `Core` is a structure that implements `zap.Core` interface for logging into journald. You can use comments from interface definition here: https://github.com/uber-go/zap/blob/845ca51d5b8d9fed9fe14f35ab13b6b160d5762d/zapcore/core.go#L23-L44
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
syslog_fields.go Outdated
@ -0,0 +22,4 @@
const (
// Facility.
// From /usr/include/sys/syslog.h.
// These are the same up to LogFtp on Linux, BSD, and OS X.
Owner

These comments belongs to Facility type.

// Facility defines syslog facility from /usr/include/sys/syslog.h
type Facility uint32 

// Facilities before LogFtp are the same on Linux, BSD, and OS X.
const (
	LogKern Facility = iota << 3
	LogUser
    ...
)
These comments belongs to Facility type. ```go // Facility defines syslog facility from /usr/include/sys/syslog.h type Facility uint32 // Facilities before LogFtp are the same on Linux, BSD, and OS X. const ( LogKern Facility = iota << 3 LogUser ... ) ```
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
syslog_fields.go Outdated
@ -0,0 +49,4 @@
LogLocal7
)
const LogFacmask = 0x03f8 /* mask to extract facility part */
Owner
// LogFacmask is used to extract facility part of the message.
const LogFacmask = 0x03f8

or something like that.

```go // LogFacmask is used to extract facility part of the message. const LogFacmask = 0x03f8 ``` or something like that.
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
syslog_fields.go Outdated
@ -0,0 +51,4 @@
const LogFacmask = 0x03f8 /* mask to extract facility part */
var SyslogFields = []string{
Owner

Add a comment that this is a list of default indexed syslog fields

// SyslogFields contains slice of fields that
// indexed with syslog by default.
var SyslogFields = []string{...}
Add a comment that this is a list of default indexed syslog fields ```go // SyslogFields contains slice of fields that // indexed with syslog by default. var SyslogFields = []string{...} ```
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
dkirillov requested review from storage-services-developers 2023-04-04 14:50:08 +00:00
dkirillov requested review from storage-services-committers 2023-04-04 14:50:08 +00:00
vdomnich-yadro reviewed 2023-04-04 15:45:12 +00:00
vdomnich-yadro left a comment
Member

I think @alexvanin nailed it. Not much to add from my side.

I think @alexvanin nailed it. Not much to add from my side.
@ -0,0 +108,4 @@
}
}
// getFieldValue - return interface-value of filled property of zapcore.Field
Member
// getFieldValue returns underlying value stored in zapcore.Field.

Go style suggests to format comments as proper sentences.
And linter prefers them with the period in the end :)

``` // getFieldValue returns underlying value stored in zapcore.Field. ``` Go style suggests to format comments as proper sentences. And linter prefers them with the period in the end :)
Author
Member

fixed

fixed
AlekseySVTN marked this conversation as resolved
dstepanov-yadro reviewed 2023-04-05 08:32:08 +00:00
Readme.md Outdated
@ -0,0 +18,4 @@
"go.uber.org/zap/zapcore"
)
func main() {

What do you think about moving the example from README.md to a separate main.go file?

What do you think about moving the example from README.md to a separate main.go file?
First-time contributor

+1.
There is an example of example :) in the frostfs-node project: it stores sample configs in dedicated folder https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/config/example

+1. There is an example of example :) in the `frostfs-node` project: it stores sample configs in dedicated folder https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/config/example
Author
Member

Before we have started to create library, we with Alex Vanin, Vladimir Domnich and Evgeniy Stratonikov decided to exclude main.go or example/example.log from library. But for me, it seems to be good to have something like that. So, I think, we should discuss it in new issue: #6

Before we have started to create library, we with Alex Vanin, Vladimir Domnich and Evgeniy Stratonikov decided to exclude main.go or example/example.log from library. But for me, it seems to be good to have something like that. So, I think, we should discuss it in new issue: https://git.frostfs.info/TrueCloudLab/zapjournald/issues/6
dstepanov-yadro marked this conversation as resolved
AlekseySVTN force-pushed feature/0-initial_implementation from 991e673754 to 074fe474dd 2023-04-10 12:36:00 +00:00 Compare
Author
Member

@alexvanin , @vdomnich-yadro , @ironbee , @fyrchik , @dstepanov-yadro , @dkirillov
Please, look at this again, when you will have a time.

@alexvanin , @vdomnich-yadro , @ironbee , @fyrchik , @dstepanov-yadro , @dkirillov Please, look at this again, when you will have a time.
dstepanov-yadro approved these changes 2023-04-10 12:55:46 +00:00
ironbee approved these changes 2023-04-10 13:26:02 +00:00
alexvanin approved these changes 2023-04-10 14:13:25 +00:00
alexvanin changed title from [#2] Initial implementation to Initial implementation 2023-04-10 14:23:17 +00:00
Owner

We still have some issues to do, but it looks like a good starting point.

We still have some issues to do, but it looks like a good starting point.
alexvanin merged commit 074fe474dd into master 2023-04-10 14:24:36 +00:00
alexvanin deleted branch feature/0-initial_implementation 2023-04-10 14:24:36 +00:00
alexvanin referenced this pull request from a commit 2023-04-10 14:24:36 +00:00
vdomnich-yadro approved these changes 2023-04-11 16:32:16 +00:00
vdomnich-yadro left a comment
Member

A = attention.

I've just noticed that I am reviewing a merged PR 😅

A = attention. I've just noticed that I am reviewing a merged PR 😅
@ -0,0 +51,4 @@
// LogFacmask is used to extract facility part of the message.
const LogFacmask = 0x03f8
// SyslogFields contains slice of fields that
Member
fields that
indexed with ...

->

fields that are
indexed by ...
``` fields that indexed with ... ``` -> ``` fields that are indexed by ... ```
Sign in to join this conversation.
No description provided.