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

Metrics from VICI client #8

Merged
merged 47 commits into from
Feb 6, 2020
Merged

Metrics from VICI client #8

merged 47 commits into from
Feb 6, 2020

Conversation

Crevil
Copy link
Member

@Crevil Crevil commented Feb 4, 2020

This change introduces prometheus metrics from a strongswan process.
It collects statistics by inspecting the results of swanctl --list-conn and swanctl --list-sas calls.

Package vici is ported from https://github.com/bronze1man/goStrongswanVici and updated with error handling and removal of fmt.Print calls.

Package strongswan implements the actually collection of data and reports to the PrometheusReporter instance.

@eXeDK
Copy link

eXeDK commented Feb 4, 2020

This is pretty cool 👍 💯

This reduces the overhead of all methods requesting an API has to converts its
payload up front.
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Show resolved Hide resolved
internal/vici/monitorSA.go Show resolved Hide resolved
@Crevil Crevil marked this pull request as ready for review February 5, 2020 13:17
Copy link
Contributor

@emilingerslev emilingerslev left a comment

Choose a reason for hiding this comment

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

Good work. Had a bit hard time of getting a full overview of the code, but it seems pretty solid 👍
I've added my proposals for the buckets

internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
internal/metrics/metrics.go Show resolved Hide resolved
internal/metrics/metrics.go Outdated Show resolved Hide resolved
@Crevil
Copy link
Member Author

Crevil commented Feb 6, 2020

I've not set any of all the labels we discussed @emilingerslev . And I've removed the notes we made when discussing it, so maybe we should give it another try?
Currently I only have child SA names on the metrics relevant for that.

@emilingerslev
Copy link
Contributor

Alright, so yeah maybe we should go over the labels again. From the top of my head I got for Child SA's

  • Local IP range
  • Remote IP range

For IKE SA's (and the Child SA's below):

  • Local Peer IP
  • Remote Peer IP

@Crevil
Copy link
Member Author

Crevil commented Feb 6, 2020

I'll add that. Then we can get it merged and out and running. Then we can add new labels later on.

@lunarway lunarway deleted a comment from kaspernissen Feb 6, 2020
@lunarway lunarway deleted a comment from HamAstrochimp Feb 6, 2020
@Crevil
Copy link
Member Author

Crevil commented Feb 6, 2020

I've added labels. An example metric is now like this.

strong_duckling_ike_sa_bytes_in_total{child_sa_name="net-net-0",ike_sa_name="gw-gw",instance="strongswan1:8000",job="strongswan1",local_ip_range="10.101.0.1/32",local_peer_ip="172.22.0.2",remote_ip_range="10.102.0.1/32",remote_peer_ip="172.22.0.3"}

@Crevil
Copy link
Member Author

Crevil commented Feb 6, 2020

@emilingerslev ready for another review of the changes.

Copy link
Contributor

@emilingerslev emilingerslev left a comment

Choose a reason for hiding this comment

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

Looks good 👍 lets merge this! 💪

@Crevil
Copy link
Member Author

Crevil commented Feb 6, 2020

Great. I'll work on a good commit message :D

@Crevil Crevil merged commit 9cf4eda into master Feb 6, 2020
@Crevil Crevil deleted the feature/vici-client branch February 6, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants