Skip to content

Update wifi stats to support multiple stations (#977) #980

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

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

neiledgar
Copy link
Contributor

Signed-off-by: neiledgar [email protected]

@@ -65,14 +65,14 @@ func NewWifiCollector() (Collector, error) {
)

var (
labels = []string{"device"}
labels = []string{"device", "hw"}
Copy link
Member

Choose a reason for hiding this comment

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

I think hw is nice and short, but not very descriptive. We generally recommend more descriptive names. How about station_address.

Copy link
Contributor Author

@neiledgar neiledgar Jun 19, 2018

Choose a reason for hiding this comment

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

From nl80211.h, it is called
* @NL80211_ATTR_MAC: MAC address (various uses)

On linux iw dev wlan1 station dump command output, it is not explicity named.

I think station_address might be a bit repetitive given the statistics are named node_station_ but any of the following would be fine:

mac
hardware_address
hw_address
or even just address

Would one of these be acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

True, how about mac_address, to match the datasource.

Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM after changing the label name to mac_address.

@neiledgar neiledgar force-pushed the multiple_stations_support branch from b57d682 to 8da695f Compare June 20, 2018 10:28
@neiledgar
Copy link
Contributor Author

neiledgar commented Jun 20, 2018

Changed hw label to mac_address. I squashed all the changes into one commit. Apologies if I lost your comments. Is the correct workflow to add additional commits rather than squash all review updates into one commit?

@SuperQ
Copy link
Member

SuperQ commented Jun 20, 2018

@neiledgar Squashing commits is fine.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@discordianfish
Copy link
Member

LGTM

@SuperQ SuperQ merged commit 7e4d9bd into prometheus:master Jul 16, 2018
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
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.

4 participants