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

Added firewall.rules.hash metric #1010

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

britcey
Copy link
Contributor

@britcey britcey commented Nov 10, 2023

Added a FNV-1 hash of the firewall rules as a Prometheus value.

Added a FNV-1 hash of the firewall rules as a Prometheus value.
Copy link

Thanks for the contribution! Before we can merge this, we need @britcey to sign the Salesforce Inc. Contributor License Agreement.

@britcey
Copy link
Contributor Author

britcey commented Nov 10, 2023

Closes issue #1005

@nbrownus
Copy link
Collaborator

Thanks for the PR! My primary concern is that there is no direct correlation back to the log line. I think this could be resolved in two ways.

  1. Include the FNV hash in the log lines
  2. Include the sha256 hash as a label

Ideally we would do both here but option 2 is more difficult so lets add the FNV hash to the log lines.

firewall.go Show resolved Hide resolved
@britcey
Copy link
Contributor Author

britcey commented Nov 14, 2023

Thanks for the PR! My primary concern is that there is no direct correlation back to the log line. I think this could be resolved in two ways.

  1. Include the FNV hash in the log lines
  2. Include the sha256 hash as a label

Ideally we would do both here but option 2 is more difficult so lets add the FNV hash to the log lines.

Option 2 would also increase cardinality, creating a new timeseries every time that label changed, so including the FNV hash in the log is the way to go, for sure. I'll make that happen.

@britcey
Copy link
Contributor Author

britcey commented Nov 14, 2023

This work for the logs?

Startup:

level=info msg="Firewall started" firewallHashes="SHA:8cdd862ba57eca683ac4fb32fcd8f727743df3ba0c92b267c3e9ca128e43be8d,FNV:-4874977699962996863"

after a reload:

level=info msg="New firewall has been installed" firewallHashes="SHA:3775881c4dfa7c63e3d343e747ad66b0ddf6a72fde1ce3b2eea43436175f101f,FNV:7449190936218615427" oldFirewallHashes="SHA:8cdd862ba57eca683ac4fb32fcd8f727743df3ba0c92b267c3e9ca128e43be8d,FNV:-4874977699962996863" rulesVersion=1

Let go-metrics cast the uint32 to a int64, so it won't be lossy
when it eventually emits a float64 Prometheus metric.
@britcey
Copy link
Contributor Author

britcey commented Nov 18, 2023

Anything needed here?

@johnmaguire johnmaguire merged commit 01cddb8 into slackhq:master Nov 28, 2023
6 checks passed
@johnmaguire
Copy link
Collaborator

@britcey Thanks for your contribution!

@britcey britcey deleted the firewall_prometheus_hash branch November 28, 2023 20:30
@wadey wadey added this to the v1.8.0 milestone Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants