Skip to content
This repository has been archived by the owner on Feb 21, 2024. It is now read-only.

add server stats to /info endpoint #1859

Merged
merged 1 commit into from Mar 26, 2019

Conversation

seebs
Copy link
Contributor

@seebs seebs commented Feb 11, 2019

report the approximate hardware specs (CPU speed, cores, memory)
of the server in the /info endpoint. This may be useful when
benchmarking.

Fixes #651

(note: docs haven't been changed, and maybe they should, but right now we don't actually document /info at all, apparently?)

Pull request checklist

Code review checklist

This is the checklist that the reviewer will follow while reviewing your pull request. You do not need to do anything with this checklist, but be aware of what the reviewer will be looking for.

  • Ensure that any changes to external docs have been included in this pull request.
  • If the changes require that minor/major versions need to be updated, tag the PR appropriately.
  • Ensure the new code is properly commented and follows Idiomatic Go.
  • Check that tests have been written and that they cover the new functionality.
  • Run tests and ensure they pass.
  • Build and run the code, performing any applicable integration testing.

@travisturner
Copy link
Member

My initial feedback is that this doesn't follow the pattern of keeping dependencies out of the pilosa package (and instead injecting them via the server package).

See gcnotify as an example.

I don't have a strong opinion about this, but we did a lot of work to clean up the pilosa package so it might be worth the additional work to continue that pattern.

Copy link
Member

@travisturner travisturner left a comment

Choose a reason for hiding this comment

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

sorry, left my feedback in the wrong place. See my comment on the main thread of this PR regarding dependency injection.

@seebs
Copy link
Contributor Author

seebs commented Feb 14, 2019

revised to use the existing gopsutil functionality we already had handy.

Copy link
Member

@travisturner travisturner left a comment

Choose a reason for hiding this comment

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

I'm confused about how cores is determined.
On my mac, I get this for infos:

([]cpu.InfoStat) (len=1 cap=1) {
 (cpu.InfoStat) {"cpu":0,"vendorId":"GenuineIntel","family":"6","model":"61","stepping":4,"physicalId":"","coreId":"","cores":2,"modelName":"Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz","mhz":2900,"cacheSize":256,"flags":["fpu","vme","de","pse","tsc","msr","pae","mce","cx8","apic","sep","mtrr","pge","mca","cmov","pat","pse36","clfsh","ds","acpi","mmx","fxsr","sse","sse2","ss","htt","tm","pbe","sse3","pclmulqdq","dtes64","mon","dscpl","vmx","est","tm2","ssse3","fma","cx16","tpr","pdcm","sse4.1","sse4.2","x2apic","movbe","popcnt","aes","pcid","xsave","osxsave","seglim64","tsctmr","avx1.0","rdrand","f16c","smep","erms","rdwrfsgs","tsc_thread_offset","bmi1","avx2","bmi2","invpcid","smap","rdseed","adx","ipt","fpu_csds","syscall","xd","1gbpage","em64t","lahf","lzcnt","prefetchw","rdtscp","tsci"],"microcode":""}
}

which seems to indicate that I have 2 cores.
But /info returns:

{
  "shardWidth": 1048576,
  "memory": 17179869184,
  "cpuType": "Intel(R) Core(TM) i5-5287U CPU @ 2.90GHz",
  "CPUPhysicalCores": 1,
  "CPULogicalCores": 1,
  "CPUMHz": 2900
}

api.go Outdated Show resolved Hide resolved
@seebs
Copy link
Contributor Author

seebs commented Feb 14, 2019

Hah. And I added the json field names... and that broke the test, which I apparently didn't rerun.

@seebs
Copy link
Contributor Author

seebs commented Feb 14, 2019

it looks like that CI failure was just the ongoing occasional races in memberlist.

@jaffee
Copy link
Member

jaffee commented Mar 22, 2019

should we try to get this merged? needs a rebase

@seebs
Copy link
Contributor Author

seebs commented Mar 22, 2019

I think so. It's useful, and imagine uses it if it's available.

@seebs
Copy link
Contributor Author

seebs commented Mar 25, 2019

the patch applied cleanly to master, so let's see whether CI passes!

'M': 1,
'G': 1000,
'T': 1000 * 1000,
}
Copy link
Member

Choose a reason for hiding this comment

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

I did not know you could do this... not sure how I feel about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is two syntactic sugar at once. One is that you can use labeled initializers in slice/array constants, just like in struct ones, the other is that you can treat runes as their unicode values.

Very convenient, though.

jaffee
jaffee previously approved these changes Mar 25, 2019
Copy link
Member

@jaffee jaffee left a comment

Choose a reason for hiding this comment

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

approved, but... needs another rebase :)

@jaffee
Copy link
Member

jaffee commented Mar 25, 2019

Travis' issue seems to be fixed. At least... my mac reports correct numbers and ours our about the same age.

@jaffee jaffee dismissed travisturner’s stale review March 25, 2019 20:47

issue seems to be resolved

report the approximate hardware specs (CPU speed, cores, memory)
of the server in the /info endpoint. This may be useful when
benchmarking.

We do some workarounds because gopsutil's core count output is
confusingly different between Linux and Darwin, and the MHz output
is usually wrong on Linux. Intel's app notes say to just parse
the model string. Whyyyyyyy.
@seebs seebs merged commit 4955dff into FeatureBaseDB:master Mar 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add endpoint to get host/infrastructure info from Pilosa
3 participants