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

Implement default process & runtime metrics #61

Closed
wants to merge 43 commits into from

Conversation

MrLotU
Copy link
Contributor

@MrLotU MrLotU commented Feb 15, 2020

Fixes #53

Currently only implemented on Linux (still figuring out how to handle this on MacOS), and on a 2 second loop.

Points for discussion:

  • Should we move forward without a MacOS implementation since it's usually not used in production environments
  • Should the process metric collection be run on a loop, or be requested at-will by the Metrics Backend
  • Is the 2 second delay between loop runs okay
  • How can we validate these values. Current implementation is copied from the python library

@swift-server-bot
Copy link

Can one of the admins verify this patch?

3 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

thank you so much @MrLotU, some comments

Copy link
Member

@ddossot ddossot left a comment

Choose a reason for hiding this comment

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

Great stuff!

Sources/CoreMetrics/SystemMetrics.swift Outdated Show resolved Hide resolved
Sources/CoreMetrics/SystemMetrics.swift Outdated Show resolved Hide resolved
Sources/CoreMetrics/SystemMetrics.swift Outdated Show resolved Hide resolved
Sources/CoreMetrics/SystemMetrics.swift Outdated Show resolved Hide resolved
@tomerd
Copy link
Member

tomerd commented Feb 21, 2020

also note the interesting work the perfect folks did in https://github.com/PerfectlySoft/Perfect-SysInfo

@MrLotU MrLotU requested a review from tomerd July 28, 2020 12:23
@ktoso ktoso changed the base branch from master to main August 20, 2020 00:21
@tomerd
Copy link
Member

tomerd commented Sep 18, 2020

heya @MrLotU, is this ready for review?

@MrLotU
Copy link
Contributor Author

MrLotU commented Sep 18, 2020

Yup. Have a couple of outstanding questions, but I think we're in a good place 😄

@MrLotU
Copy link
Contributor Author

MrLotU commented Oct 9, 2020

@tomerd Any update on this? It's been stale for quite a while 😄

@tomerd
Copy link
Member

tomerd commented Oct 9, 2020

Hi @MrLotU thank you for your patience with this. @drexin @ktoso and myself have been debating how to best integrate this work - if it belongs as part of the swift-metrics repo or an extension to it. At this time, we feel it would best if we did not include this in the swift-metrics repo so it remains focused on the API and not collection or aggregation of metrics. Instead, we think we should create and move this code to a separate repo that provides additional metrics functionality. We recognize this means we need to change the integration again since it is using internal APIs that we would potentially need to expose for extensibility.

WDYT about the approach? a good next step could be to extract this to a separate module within swift-metrics and see what the integration could look like. That should give us an indication as to the final shape APIs changes needed in swift-metrics. If you prefer, I am happy to spend time adding some commits on top of this PR to that end.

@MrLotU
Copy link
Contributor Author

MrLotU commented Oct 16, 2020

No problem at all. I can see your points for moving this into a separate project and agree this would be a great way to move forward.

I will hopefully have some time over the coming weekend to get a first draft up as a new module within swift-metrics to get an idea of what public API is needed 👍

@MrLotU
Copy link
Contributor Author

MrLotU commented Nov 6, 2020

Took me a bit longer to find the time, but I've migrated all of SystemMetrics to a new module.

The only change I made to the CoreMetrics module is making the lock public and making withWriterLock public. However this might not be required if SystemMetrics would ship with it's own lock implementation.

All other functionality & API stays the same

@MrLotU MrLotU requested a review from drexin November 6, 2020 14:26
@tomerd
Copy link
Member

tomerd commented Nov 9, 2020

thanks @MrLotU, how about instead of making the Lock public we expose withWriterLock and withReaderLockon the metrics directly? this will help not leak the Lock type which we prefer to keep internal. Then we can call self.withWriterLock instead of self.lock.withWriterLock etc?

@MrLotU
Copy link
Contributor Author

MrLotU commented Nov 9, 2020

That'd be a workable solution 👍 Will implement

@ktoso
Copy link
Member

ktoso commented Nov 10, 2020

Awesome work @MrLotU -- giving it a final review round then and let's find it a home... :)

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Discussed with Tom that maybe we can quickly spin out an swift-metrics-extras to land this in. Other than where to merge it into this looks good, thanks a lot for the prolonged effort @MrLotU !

@ktoso
Copy link
Member

ktoso commented Nov 14, 2020

Thank you very much @MrLotU!

This is now released as part of the new swift metrics extras package! 🎉 🎉 🎉

Closing the PR here then, Thanks again!

@ktoso ktoso closed this Nov 14, 2020
@tomerd
Copy link
Member

tomerd commented Nov 14, 2020

thank you @MrLotU

@MrLotU
Copy link
Contributor Author

MrLotU commented Nov 14, 2020

Thanks @tomerd and @ktoso for the continued help & guidance. Super happy with how this turned out 😄

@MrLotU MrLotU deleted the LotU-ProcessMetrics branch November 14, 2020 12: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.

Export default process & runtime metrics
9 participants