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

feat(probe): Fortiswitch port stats #219

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

Jurka007
Copy link

@bluecmd
Copy link
Collaborator

bluecmd commented Jul 17, 2023

@Jurka007 can you rebase and fix the check errors?

@bluecmd bluecmd changed the title Fortiswitch port stats feat(probe): Fortiswitch port stats Jul 18, 2023
@Jurka007 Jurka007 force-pushed the fortiswitch_port_stats branch from 4d6c403 to 9d0a753 Compare July 24, 2023 06:13
Copy link
Collaborator

@bluecmd bluecmd left a comment

Choose a reason for hiding this comment

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

Some metric naming and label comments

README.md Outdated
* `fortiswitch_port_receive_drops_total`
* `fortiswitch_port_receive_oversized_packets_total`
* _SwitchController/ManageSwitch/Health_
* `fortiswitch_health_summary_cpu`
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Avoid using words like mem when memory works
  • These health metrics names have no unit in them (e.g. add _celsius tofortiswitch_health_temperature

Copy link
Author

Choose a reason for hiding this comment

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

Actually the unit label is part of the metric
fortiswitch_health_temperature{VDOM="root", fortiswitch="FSS123456789", module="sensor1(CPU Board Temp)", unit="celsius"} 45

VDOM string
}
var r swResponse
//var r map[string]swResponse
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove dead code

mSumCPU = prometheus.NewDesc(
"fortiswitch_health_summary_cpu",
"Summary CPU health",
[]string{"rating", "fortiswitch", "VDOM"}, nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lowercase all labels

//for _, sw := range r {
for fswitch, hr := range r.Results {

m = append(m, prometheus.MustNewConstMetric(mSumCPU, prometheus.GaugeValue, hr.Summary.CPU.Value, hr.Summary.CPU.Rating, fswitch, r.VDOM))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The rating likely shouldn't be a level ä label but rather a metric like "xxx_is_good". That, or an enum label on a _rating metric. Otherwise it's really hard to use it in alerts.

Copy link
Author

Choose a reason for hiding this comment

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

Right, we don't care about the numbers as there is no documentations what those really means. The fortios has own logic to either return good or not good, but no idea what are the other ratings.
Perhaps the alert should be based on any other than good value.

Copy link
Author

Choose a reason for hiding this comment

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

wait, prometheus has only numeric metrics.. 🤔

)
mSumUpTime = prometheus.NewDesc(
"fortiswitch_health_summary_uptime",
"Summary Uptime",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have the documentation it would be really great if these descriptions could be made more verbose so I for example have any idea what these means. But if you don't, it's fine - sometimes we simply don't know

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately there is nothing in the docs :( I have no idea either, we can only guess..

Copy link
Author

Choose a reason for hiding this comment

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

Seems to be a summary of the overall status, good or not good 🤷

}
m = append(m, prometheus.MustNewConstMetric(mPortStatus, prometheus.GaugeValue, pStatus, swr.VDOM, swr.Name, pi.Interface, pi.Vlan, pi.Duplex))
m = append(m, prometheus.MustNewConstMetric(mPortSpeed, prometheus.GaugeValue, pi.Speed*1000*1000, swr.VDOM, swr.Name, pi.Interface, pi.Vlan, pi.Duplex))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having duplex as a label seems a bit off to me. Why not have it as a separate metric?

Copy link
Author

Choose a reason for hiding this comment

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

The value is either half or full. I am fine with that.
No big deal to add, but what values would you expect? 1 for full and 0.5 for half

if swr.Status == "Connected" {
swStatus = 1.0
}
m = append(m, prometheus.MustNewConstMetric(mSwitchStatus, prometheus.GaugeValue, swStatus, swr.VDOM, swr.Name, swr.FgPeerIntfName, swr.Connection, swr.State))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The state label here should likely be an enum label if the possible values of state is limited and known. It's hard to use it for anything useful otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

State: Authorization state of the FortiSwitch

Discovered, DeAuthorized, Authorized

mPortStatus = prometheus.NewDesc(
"fortiswitch_port_status",
"Whether the switch port is up or not",
[]string{"vdom", "name", "interface", "vlan", "duplex"}, nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can a port only have one VLAN on a
Fortiswitch? That seems odd. What happens when the port is a VLAN trunk?

Copy link
Author

Choose a reason for hiding this comment

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

These values seems to correspond to port switch view https://<fg-ip>:<fg-port>/ng/switchctrl/portsso the vlan is Native VLAN per single port. There is no info about trunk ports.

mSwitchStatus = prometheus.NewDesc(
"fortiswitch_status",
"Whether the switch is connected or not",
[]string{"vdom", "name", "fgt_peer_intf_name", "connection_from", "state"}, nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This meric is not like the rest. The fgt_peer_intf_name and connection_from is not clear to me what this metric is. Care to add it to the description or as a comment in code?

@Jurka007
Copy link
Author

Jurka007 commented Mar 5, 2025

I am back baby. I don't remember what I have done here :) Will try to update asap so we can merge this.

@Jurka007
Copy link
Author

Jurka007 commented Mar 6, 2025

🤦
GET api/v2/monitor/switch-controller/managed-switch is deprecated. It will be removed in 7.2. It's replaced by /api/v2/monitor/switch-controller/managed-switch/status for faster performance.

GET api/v2/monitor/switch-controller/managed-switch/health to be deprecated in FortiOS v7.6.0, replaced by /api/v2/monitor/switch-controller/managed-switch/health-status.

@Jurka007
Copy link
Author

Jurka007 commented Mar 6, 2025

Btw the #215 has the same api call. didn't notice until now
so it's kinda duplicate 🤦

@Jurka007 Jurka007 force-pushed the fortiswitch_port_stats branch from f31a005 to d1fbc91 Compare March 6, 2025 17:12
@Jurka007 Jurka007 requested a review from bluecmd March 7, 2025 07:39
@Jurka007 Jurka007 force-pushed the fortiswitch_port_stats branch from e0688f2 to 5ad6d03 Compare March 7, 2025 08:58
@Jurka007
Copy link
Author

Jurka007 commented Mar 7, 2025

Ok, this should be it. Added support for new api changes. Didn't test yet, don't have any firewall with os 7.6

Also maybe worth to merge the managed_switch probe and this one.

Jurka007 and others added 19 commits March 7, 2025 14:22
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
Signed-off-by: jiri <[email protected]>
* fix(BGP Info): Added support for bgp session state as value next to label

* test(bgp): Updated tests to account for new value

* test(bgp): Also change metric description in tests

---------

Co-authored-by: Gianni Stubbe <[email protected]>
Signed-off-by: jiri <[email protected]>
* docs: Updated descriptions of metrics to be self-explanatory

* test(interface): Fixed tests for system and ipsec interfaces

---------

Co-authored-by: Gianni Stubbe <[email protected]>
Signed-off-by: jiri <[email protected]>
* feat(ippool): Added support for ippool info

This merge adds support for ippool information. This closes prometheus-community#231

* fix(naming): Updated metric names to be in line with prometheus conventions

* test(ippool): Added tests for the firewall ippool feature

* style: Updated tests and readme to align for new naming

* style: Changed metric name to match other percentage metrics

* fix(ippool): Updated value percentage to be 0-1.0

---------

Co-authored-by: Gianni Stubbe <[email protected]>
….0 (prometheus-community#250)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: jiri <[email protected]>
Signed-off-by: Christian Svensson <[email protected]>
Signed-off-by: jiri <[email protected]>
…ool types (prometheus-community#267)

* feat: Added pba_per_ip return for ippool to differentiate between types of ippools
* docs: Updated readme

---------

Co-authored-by: Gianni Stubbe <[email protected]>
….0 (prometheus-community#263)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: jiri <[email protected]>
renovate bot and others added 6 commits March 7, 2025 14:29
…ty#260)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: jiri <[email protected]>
In order for this project to be adopted by the Prometheus Community
update the license to Apache Version 2.0.

Signed-off-by: SuperQ <[email protected]>
Signed-off-by: jiri <[email protected]>
* Update for Prometheus Community

Update various files for Prometheus Community
* Use Prometheus common Makefile setup.
* Add license headers in Go files.
* Update Go module path.
* Enable dependabot.
* Update GitHub Actions.
* Fixup golangci-lint errors.

---------

Signed-off-by: SuperQ <[email protected]>
@Jurka007 Jurka007 force-pushed the fortiswitch_port_stats branch from 3f339fd to 64575ff Compare March 7, 2025 13:32
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.

6 participants