-
Notifications
You must be signed in to change notification settings - Fork 78
+Instrumentation points and PoC os_signpost (Instruments) instrument #475
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
Conversation
| @@ -0,0 +1,195 @@ | |||
| <?xml version="1.0" encoding="UTF-8" ?> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual instrument definition; it relates to the os_signpost calls we do in the code
| self.address = shell.address | ||
| self.signpostID = OSSignpostID( | ||
| log: InstrumentsActorInstrumentation.logActors, | ||
| object: shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to this the lifetime of this object is related to the specific shell <3 And all traces show up as related to it.
We cannot use the Address as this needs to be a pointer (i.e. a class value)
| log: InstrumentsActorInstrumentation.logActors, | ||
| name: "Actor Lifecycle", | ||
| signpostID: self.signpostID, | ||
| "actor-spawned,myself:%{public}@", "\(self.address)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These get (efficiently) parsed and carried into the Instruments app.
TODO: protect with if myLoggingHandle.signpostsEnabled { to only serialize the address and other data when needed.
| log: InstrumentsActorInstrumentation.logActors, | ||
| name: "Actor Messages", | ||
| signpostID: self.signpostID, | ||
| "actor-message-received,myself:%{public}@,type:%{public}s", "\(self.address)", String(reflecting: type(of: message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: protect with if myLoggingHandle.signpostsEnabled { to only serialize the address and other data when needed.
TODO: carry actual message as well
| settings.configure(with: OSSignpostInstrumentationProvider()) | ||
| #endif | ||
| return settings | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default on Apple platforms we enable the signpost instrumentation, it's pretty cheap and if someone does not want it they may disable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instrumentation always has a cost though? might be better to disable by default and enable as needed?
|
More than happy to review this in depth (!). I'll merge to ease development and using this to measure ongoing work. |
| @@ -0,0 +1,171 @@ | |||
| // !$*UTF8*$! | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to check in these .xcodeproj files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -0,0 +1,14 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Username in file path ^
| <?xml version="1.0" encoding="UTF-8" ?> | ||
| <package> | ||
| <id>com.apple.actors.ActorInstruments</id> | ||
| <version>0.4.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does version have any special implications? As in, this has to match project version or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has one extra meaning:
if you "install" instruments into your Instruments.app "newer version" overrides an old one I believe. So when we update this we could keep updating this like 0.4.0.1 perhaps? And then bump as well whenever we release a tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| settings.configure(with: OSSignpostInstrumentationProvider()) | ||
| #endif | ||
| return settings | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instrumentation always has a cost though? might be better to disable by default and enable as needed?
Motivation:
Related materials:
So we're actaually just on time with being able to do these things, it's a recent feature :-)
Modifications:
Instruments.appthat shows actor lifecycle, messages and other eventsThe naming is a bit of a mess... need to figure it out.
Generally:
Result: