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.

Sign up for GitHub

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

Jump to bottom

add diskstats on Darwin #593

Merged
merged 8 commits into from
Jul 6, 2017
Merged

add diskstats on Darwin #593

merged 8 commits into from
Jul 6, 2017

Conversation

lufia
Copy link
Contributor

@lufia lufia commented Jun 3, 2017

I have implemented a part of diskstats metrics.

  • reads_completed
  • sectors_read
  • read_time_ms
  • writes_completed
  • sectors_written
  • write_time_ms
  • bytes_read
  • bytes_written

but below metrics are not implemented.

  • reads_merged
  • writes_merged
  • io_now
  • io_time_ms
  • io_time_weighted

grobie
grobie reviewed Jun 6, 2017
View reviewed changes
collector/diskstats_darwin.go Outdated
{
typedDesc: typedDesc{
desc: prometheus.NewDesc(
prometheus.BuildFQName(Namespace, diskSubsystem, "read_time_ms"),
Copy link
Member

Choose a reason for hiding this comment

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

These metric names don't follow our best practices (base unit for time is seconds, counters should end on _total). I guess we need to decide whether we want to stay consistent with the current linux implementation, or avoid a breaking change for darwin users along the road. Eventually, we'll rename the linux metrics in a big breaking release.

I think I'd use better names here already.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @grobie, all new metrics should follow the best practices (unless maybe all the exporter is doing is surfacing existing metrics name).

discordianfish
discordianfish requested changes Jun 6, 2017
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me in general, but the metrics names need changing.

collector/diskstats_darwin.go Outdated
{
typedDesc: typedDesc{
desc: prometheus.NewDesc(
prometheus.BuildFQName(Namespace, diskSubsystem, "read_time_ms"),
Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @grobie, all new metrics should follow the best practices (unless maybe all the exporter is doing is surfacing existing metrics name).

@lufia
Copy link
Contributor Author

lufia commented Jun 8, 2017

I have renamed metrics to fit guideline

discordianfish
discordianfish approved these changes Jun 8, 2017
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@SuperQ
Copy link
Member

SuperQ commented Jun 8, 2017

I don't see an update to the vendor.json. You need to use the govendor utility to create the vendoring.

@discordianfish
Copy link
Member

Hrmm but why did the build pass then?

@SuperQ
Copy link
Member

SuperQ commented Jun 8, 2017

go build doesn't care about the json file.

@discordianfish
Copy link
Member

I thought we make it never fetch but not sure where I got that from..

@lufia
Copy link
Contributor Author

lufia commented Jun 9, 2017

sorry, since I hadn't found vendor.json until today,
I put iostat package manually in vendor directory.
I'm going to fix vendor.json.

@grobie
Copy link
Member

grobie commented Jun 9, 2017 via email

@lufia
Copy link
Contributor Author

lufia commented Jun 9, 2017

vendor.json is updated.

@grobie thank you for your mention.

SuperQ
SuperQ approved these changes Jun 9, 2017
View reviewed changes
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

@lufia
Copy link
Contributor Author

lufia commented Jul 3, 2017

@grobie What would this PR require next action?

grobie
grobie reviewed Jul 3, 2017
View reviewed changes
Copy link
Member

@grobie grobie left a comment

Choose a reason for hiding this comment

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

One last comment, looks good otherwise.

README.md Outdated
@@ -24,7 +24,7 @@ Name | Description | OS
arp | Exposes ARP statistics from `/proc/net/arp`. | Linux
conntrack | Shows conntrack statistics (does nothing if no `/proc/sys/net/netfilter/` present). | Linux
cpu | Exposes CPU statistics | Darwin, Dragonfly, FreeBSD, Linux
diskstats | Exposes disk I/O statistics from `/proc/diskstats`. | Linux
diskstats | Exposes disk I/O statistics from `/proc/diskstats`. | Darwin, Linux
Copy link
Member

Choose a reason for hiding this comment

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

That description is not longer correct, Darwin doesn't have a proc filesystem. Either drop it or describe each source.

@discordianfish
Copy link
Member

Awesome, thanks a lot for you contribution! 🎆

@discordianfish discordianfish merged commit a077024 into prometheus:master Jul 6, 2017
@lufia lufia deleted the feature/diskstat_darwin branch September 13, 2017 06:32
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@lufia @oblitorum
add diskstats on Darwin (prometheus#593)
c53c559 
* Add diskstats collector for Darwin

* Update year in the header

* Update README.md

* Add github.com/lufia/iostat to vendored packages

* Change stats to follow naming guidelines

* Add a entry of github.com/lufia/iostat into vendor.json

* Remove /proc/diskstats from description
tamcore pushed a commit to gitgrave/node_exporter that referenced this pull request Oct 22, 2024
@dependabot
Bump golang.org/x/sys from 0.13.0 to 0.15.0 (prometheus#593)
534069e 
Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.13.0 to 0.15.0.
- [Commits](golang/sys@v0.13.0...v0.15.0)

---
updated-dependencies:
- dependency-name: golang.org/x/sys
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grobie grobie grobie left review comments

@SuperQ SuperQ SuperQ approved these changes

@discordianfish discordianfish discordianfish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lufia @SuperQ @discordianfish @grobie