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

Export default process & runtime metrics #53

Open
MrLotU opened this issue Nov 10, 2019 · 5 comments
Open

Export default process & runtime metrics #53

MrLotU opened this issue Nov 10, 2019 · 5 comments
Labels
kind/enhancement Improvements to existing feature.

Comments

@MrLotU
Copy link
Contributor

MrLotU commented Nov 10, 2019

From the Prometheus client library spec, Client libraries SHOULD offer what they can of the Standard exports, documented below.

Metric name Help string Unit
process_cpu_seconds_total Total user and system CPU time spent in seconds. seconds
process_open_fds Number of open file descriptors. file descriptors
process_max_fds Maximum number of open file descriptors. file descriptors
process_virtual_memory_bytes Virtual memory size in bytes. bytes
process_virtual_memory_max_bytes Maximum amount of virtual memory available in bytes. bytes
process_resident_memory_bytes Resident memory size in bytes. bytes
process_heap_bytes Process heap size in bytes. bytes
process_start_time_seconds Start time of the process since unix epoch in seconds. seconds

I think these metrics would be useful to have, also outside of Prometheus.

My proposal is to implement these metrics into swift-metrics directly, with the option of switching them off.

Possible API would be:

MetricsSystem.bootstrap(_ factory: MetricsFactory, provideSystemMetrics: Bool = true)

As a note I'd like to add I'm not 100% sure which of the above metrics are feasible and possible to get a hold of in Swift, so some experimentation/research might be needed there.

@tomerd
Copy link
Member

tomerd commented Feb 10, 2020

this is a cool idea @MrLotU, would you like to take on proposing an implementation?

@ktoso
Copy link
Member

ktoso commented Feb 10, 2020

Sounds good, I suppose this could be done as extra module like "SystemMetrics" and added via MetricsSystem.bootstrapSystemMetrics, so it's not necessarily part of core?

Another tricky bit is labels -- this keeps coming up to be honest. I.e. one systems like to report like system.bla.bla while in others . is illegal... What I do in my libraries where I emit metrics is offering the segmentSeparator to be configurable. we migth need to do the same here, as prom wants _ but others may need .... OR we make all labels configurable, could also be good hm

Gathering those may need some execution context? 🤔

Would be nice to figure this out and be able to offer it! Looking forward to more details :)

@tomerd
Copy link
Member

tomerd commented Feb 12, 2020

Sounds good, I suppose this could be done as extra module like "SystemMetrics" and added via MetricsSystem.bootstrapSystemMetrics, so it's not necessarily part of core?

I think separate module sounds good. not sure how to exactly tie it into bootstrap, so API ideas would be great to discuss

Another tricky bit is labels -- this keeps coming up to be honest. I.e. one systems like to report like system.bla.bla while in others . is illegal... What I do in my libraries where I emit metrics is offering the segmentSeparator to be configurable. we migth need to do the same here, as prom wants _ but others may need .... OR we make all labels configurable, could also be good hm

imo, the restriction logic on labels belong with the collector, not the emitter. iow collectors for backends that are sensitive to certain characters (like Prometheus and dots) should sanitize before sending to the backend. if we put this kind of logic on the generic emission API we would need to worry about all the potential disallowed characters in all the possible backends and this is not scalable. happy to consider suggestions that show otherwise.

Would be nice to figure this out and be able to offer it! Looking forward to more details :)

+1 me too, this would be a great addition imo

@MrLotU
Copy link
Contributor Author

MrLotU commented Feb 12, 2020

I'll get some work in over the weekend. Excited to see what we can do here 😄

@ktoso
Copy link
Member

ktoso commented Feb 12, 2020

imo, the restriction logic on labels belong with the collector, not the emitter. iow collectors for backends that are sensitive to certain characters (like Prometheus and dots) should sanitize before sending to the backend.

Hmm, that is a fair point -- we could indeed handle this in the Prom library, by optionally providing some label replacements chars... This way users of apps who pick prom know "i have a lib which does use . but I use prometheus, so it should be _" or similar. Does indeed sound like the right place to put it... Doing it in my library which should have been agnostic to what backend we emit to felt kind of backwards -- I'll open a ticket there: swift-server/swift-prometheus#29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants