Adding support for /proc/net/xfrm_stat for xfrm statistics #49

pid1co · 2017-05-15T21:57:48Z

xfrm is an IP framework for transforming packets. This framework is used
to implement the ipsec protocol. This is part of the iproute2 utilities

More info can be found

xfrm is an IP framework for transforming packets. This framework is used to implement the ipsec protocol. This is part of the iproute2 utilities More info can be found * http://kernelspec.blogspot.com/2014/10/ipsec-implementation-in-linux-kernel.h/tml * http://man7.org/linux/man-pages/man8/ip-xfrm.8.html

mdlayher · 2017-05-16T02:30:09Z

xfrm.go

+		return x, err
+	}
+
+	ioFormat := "XfrmInError %d\nXfrmInBufferError %d\nXfrmInHdrError %d\n" +


I'll leave it up to the maintainers, but IMO it would be much more maintainable to use a bufio.Scanner and then map each field name to a struct field, discarding unknown fields as iteration continues.

This could look something like: https://play.golang.org/p/9UiIl8_j13.

Sounds good to me, as it will be more resilient to different file formats, e.g. having additional / less / reordered lines.

grobie

Thanks, looks great on first look.

Besides the concerns about using scanf, please include our Apache header at the top of all files (I need to fix some other files). Another small nit, our prometheus project style requirement is to write all comments as full sentence including a final period.

*Added apache license headers *Ended comments with period in accordance with documentation style

pid1co · 2017-05-17T23:19:42Z

I see some PR's people add themselves to the AUTHORS.MD file. Is that something I should do as part of this PR?

SuperQ · 2017-05-18T05:03:18Z

No, we do not use an AUTHORS file anymore.

grobie · 2017-05-18T11:15:26Z

xfrm.go

+// limitations under the License.
+
+// Package procfs provides functions to retrieve system, kernel and process
+// metrics from the pseudo-filesystem proc.


The package should only get documented once, I believe we do this already in proc.go. Most projects use a doc.go for that.

grobie

Looks good on first glance, will have a closer look tonight.

grobie · 2017-05-18T11:17:04Z

xfrm.go

+}
+
+// NewXfrmStat reads the xfrm_stat statistics.
+func NewXfrmStat() (XfrmStat, error) {


The current consensus goes towards not offering such helper methods. Let me know if you feel very strongly about this, otherwise let's remove this function.

I guess I am not seeing the point of diverging from how most of the other plugins are written. I was following the example that was set from other plugins in the repo. Is there an example of what you would prefer the format to be now?

I see your point, let's leave it like this for now. We can change everything together at a later point.

grobie · 2017-05-18T11:18:15Z

xfrm.go

+	return fs.NewXfrmStat()
+}
+
+// ParseXfrmStat reads the xfrm_stat statistics from the 'proc' filesystem.


Doc string doesn't match method name. @mdlayher also proposed to drop the New prefix and just use XfrmStat().

mdlayher · 2017-05-18T15:54:51Z

xfrm.go

+
+	for s.Scan() {
+		fields := strings.Fields(s.Text())
+		name := fields[0]


Note that you have to check if len(fields) != 2 before accessing slice elements or else it may panic.

mdlayher · 2017-05-18T15:56:04Z

xfrm.go

+
+		switch name {
+		case "XfrmInError":
+			x.XfrmInError, err = strconv.Atoi(value)


I would move the strconv.Atoi(value) outside of the switch to avoid all the repetitive lines.

Note that my example also removed the switch statement from the body of this function to keep it more concise.

If i move Atoi outside of switch statement, I will still be repeating whatever function i capture this logic under. I dont see the benefit.

Doing the check once so you can do:

x.Field = value

... is much cleaner than:

x.Field, err = strconv.Atoi(value)

corrected docstring added check for fields length to prevent panic

grobie

Thank you, looks good! Some very minor comments (the Prometheus project has a reputation of being a bit picky about style consistency ;-) ).

grobie · 2017-05-18T20:41:51Z

xfrm.go

+}
+
+// NewXfrmStat reads the xfrm_stat statistics.
+func NewXfrmStat() (XfrmStat, error) {


I see your point, let's leave it like this for now. We can change everything together at a later point.

grobie · 2017-05-18T20:43:03Z

xfrm.go

+
+// NewXfrmStat reads the xfrm_stat statistics from the 'proc' filesystem.
+func (fs FS) NewXfrmStat() (XfrmStat, error) {
+


Nit: For style consistency with the other methods, let's remove this newline here and on top of if err != nil below.

grobie · 2017-05-18T20:44:00Z

xfrm.go

+
+		if len(fields) != 2 {
+			return XfrmStat{}, fmt.Errorf(
+				"Couldnt parse %s line %s", file.Name(), s.Text())


Go's style guide suggests to not use capitalized error messages https://github.com/golang/go/wiki/CodeReviewComments#error-strings.

grobie · 2017-05-18T20:44:35Z

xfrm.go

+			x.XfrmAcquireError = value
+		}
+
+		if err != nil {


Let's move this above right below the Atoi call.

grobie · 2017-05-18T20:44:56Z

xfrm_test.go

+
+func TestXfrmStats(t *testing.T) {
+	xfrmStats, err := FS("fixtures").NewXfrmStat()
+


Nit: remove.

grobie · 2017-05-18T20:47:29Z

xfrm.go

+	XfrmOutPolDead int
+	// Policy Error
+	XfrmOutPolError     int
+	XfrmFwdHdrError     int


Is there a reason these three fields don't have a comment?

I was unable to find documentation as to what they mean. XFRM is notoriously bad at documentation. The fields with comments are taken from the proc man page

grobie · 2017-05-19T19:08:27Z

Thanks a lot for your contribution!

…s#49) * Adding support for /proc/net/xfrm_stat for xfrm statistics xfrm is an IP framework for transforming packets. This framework is used to implement the ipsec protocol. This is part of the iproute2 utilities More info can be found * http://kernelspec.blogspot.com/2014/10/ipsec-implementation-in-linux-kernel.h/tml * http://man7.org/linux/man-pages/man8/ip-xfrm.8.html * converted to bufio.Scanner / switch statement. *Added apache license headers *Ended comments with period in accordance with documentation style * removing package documentation as its redundant corrected docstring added check for fields length to prevent panic * move atoi outside of switch statement * fixing requested formatting/style changes

fix loginuid test and documentation

mdlayher reviewed May 16, 2017

View reviewed changes

grobie reviewed May 16, 2017

View reviewed changes

converted to bufio.Scanner / switch statement.

777f8ec

*Added apache license headers *Ended comments with period in accordance with documentation style

grobie reviewed May 18, 2017

View reviewed changes

mdlayher reviewed May 18, 2017

View reviewed changes

ian_anderson added 2 commits May 18, 2017 11:05

removing package documentation as its redundant

87755d0

corrected docstring added check for fields length to prevent panic

move atoi outside of switch statement

b3c3024

grobie reviewed May 18, 2017

View reviewed changes

fixing requested formatting/style changes

b4a6349

grobie merged commit 65c1f6f into prometheus:master May 19, 2017

bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023

Merge pull request prometheus#49 from idanski/loginuid_fixes

7c19730

fix loginuid test and documentation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for /proc/net/xfrm_stat for xfrm statistics #49

Adding support for /proc/net/xfrm_stat for xfrm statistics #49

pid1co commented May 15, 2017

mdlayher May 16, 2017

grobie May 16, 2017

grobie left a comment

pid1co commented May 17, 2017

SuperQ commented May 18, 2017

grobie May 18, 2017

grobie left a comment

grobie May 18, 2017

pid1co May 18, 2017

grobie May 18, 2017

grobie May 18, 2017

mdlayher May 18, 2017

mdlayher May 18, 2017

pid1co May 18, 2017

mdlayher May 18, 2017

grobie left a comment

grobie May 18, 2017

grobie May 18, 2017

grobie May 18, 2017

grobie May 18, 2017

grobie May 18, 2017

grobie May 18, 2017

pid1co May 18, 2017

grobie commented May 19, 2017


		// NewXfrmStat reads the xfrm_stat statistics from the 'proc' filesystem.
		func (fs FS) NewXfrmStat() (XfrmStat, error) {


		func TestXfrmStats(t *testing.T) {
		xfrmStats, err := FS("fixtures").NewXfrmStat()

Adding support for /proc/net/xfrm_stat for xfrm statistics #49

Adding support for /proc/net/xfrm_stat for xfrm statistics #49

Conversation

pid1co commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie left a comment

Choose a reason for hiding this comment

pid1co commented May 17, 2017

SuperQ commented May 18, 2017

Choose a reason for hiding this comment

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie commented May 19, 2017