Skip to content

Logger metadata for duration of sampling and conversion #32

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
Merged
merged 3 commits into from
Oct 9, 2025
Merged

Logger metadata for duration of sampling and conversion #32

merged 3 commits into from
Oct 9, 2025
+30 −2

Conversation

vsarunas
Copy link
Contributor

@vsarunas vsarunas commented Oct 7, 2025

Bumps macOS support to .v13 as Swift 5.10 is present.

Adds a logger metadata duration in the logs as shown in #31

2025-10-07T09:11:49.423593 [...] ProfileRecorderSampleConversion/Sampler+Conversion.swift:61 : requesting raw samples ["raw-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.raw, "symbolicated-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.perf, "symbolizer": ProfileRecorderSampleConversion.CoreSymbolicationSymboliser, "sample-count": 3000, "time-between-samples": 10ms, "peer": [IPv4]127.0.0.1/127.0.0.1:64470]

2025-10-07T09:12:28.238699 [...] ProfileRecorderSampleConversion/Sampler+Conversion.swift:70 : raw samples complete ["raw-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.raw, "symbolizer": ProfileRecorderSampleConversion.CoreSymbolicationSymboliser, "sample-count": 3000, "peer": [IPv4]127.0.0.1/127.0.0.1:64470, "duration": 38.814310791 seconds, "symbolicated-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.perf, "time-between-samples": 10ms]

2025-10-07T09:12:36.587133 [...] ProfileRecorderSampleConversion/Sampler+Conversion.swift:97 : samples symbolicated duration ["raw-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.raw, "symbolizer": ProfileRecorderSampleConversion.CoreSymbolicationSymboliser, "sample-count": 3000, "peer": [IPv4]127.0.0.1/127.0.0.1:64470, "duration": 8.348323542 seconds, "symbolicated-samples-path": /var/folders/zx/1np75sd15ygcssjxtfnq30xc0000gn/T/WYlcor8c/samples.perf, "time-between-samples": 10ms]

@vsarunas
Copy link
Contributor Author

vsarunas commented Oct 7, 2025

Is it fine to bump to manifests to 16?

error: 'ContinuousClock' is only available in Mac Catalyst 16.0 or newer
'measure(isolation:_:)' is only available in iOS 16.0 or newer
error: 'ContinuousClock' is only available in tvOS 16.0 or newer

weissi
weissi reviewed Oct 7, 2025
View reviewed changes
Copy link
Member

@weissi weissi 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 great, but raising the deployment platform requires a little thought

Package.swift Outdated
// supported
// Linux
.macOS(.v11),
.macOS(.v13),
Copy link
Member

Choose a reason for hiding this comment

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

Is this required because that'll be "SemVer major", i.e. would require us to tag 0.4.0

Copy link
Member

Choose a reason for hiding this comment

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

If this is required, we'd need to raise all platforms accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change to use a different API for the timing; but I thought that there shouldn't be uses of these old platforms due to requirement of having Swift 5.10; which looks to be supported to require Xcode 15.3 which means a requirement for macOS Sonoma?

Copy link
Member

Choose a reason for hiding this comment

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

@FranzBusch what's the latest recommendation w.r.t. platform reqs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. swift-otel is macOS 13 / iOS 16
  2. swift-prometheus is macOS 13 / iOS 16
  3. Rest of the projects don't set a supported platform and rely on a default which is something very old.

Copy link
Member

Choose a reason for hiding this comment

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

The best option right now that both reduces the burden on adopters of this package and maintainers is to define a custom availability macro and stick it on all the APIs like it was done here apple/swift-configuration@618d19d

Copy link
Member

Choose a reason for hiding this comment

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

@FranzBusch but in this case that would also be "SemVer major", right? Because @vsarunas is adding time measurement to an implementation only. So the function right now is available everywhere and we'd need to restrict it, so it would "disappear"

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's correct. The only way around this major (or in this case minor) is to make the implementation use runtime availability checks

Copy link
Member

Choose a reason for hiding this comment

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

Good point @FranzBusch . @vsarunas , would you mind if #available(...)'ing out the measure code (or writing another func measure that uses DispatchTime.now().uptimeNanoseconds that will work on all versions)?

I don't mind if you pick if #available or adding a new measure with better availability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use NIODeadline.now() that is used in the rest of the code base instead.

TimeAmount.prettyPrint has also been merged to NIO apple/swift-nio#2504 , but it formats it differently time-between-samples=0 s vs time-between-samples=0s

weissi
weissi approved these changes Oct 9, 2025
View reviewed changes
Copy link
Member

@weissi weissi 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!

@weissi weissi added the 🔨 semver/patch No public API change. label Oct 9, 2025
@weissi
Copy link
Member

weissi commented Oct 9, 2025 •
edited
Loading

@vsarunas thank you! needs a rebase but I also enabled auto-merge. Don't know why I don't get a button to auto update the branch

@weissi weissi enabled auto-merge (squash) October 9, 2025 11:48
@weissi weissi merged commit aec12a0 into apple:main Oct 9, 2025
54 of 55 checks passed
@hassila
Copy link

hassila commented Oct 9, 2025

Don't know why I don't get a button to auto update the branch

@weissi it's a setting for the repo, you might want to update it, see https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

@weissi
Copy link
Member

weissi commented Oct 9, 2025

Don't know why I don't get a button to auto update the branch

@weissi it's a setting for the repo, you might want to update it, see https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

Hmm, I do have that enabled which I believe is the correct setting

Screenshot 2025-10-09 at 2 49 00 pm

@weissi
Copy link
Member

weissi commented Oct 9, 2025

Thanks @vsarunas , released in https://github.com/apple/swift-profile-recorder/releases/tag/0.3.7

@hassila
Copy link

hassila commented Oct 9, 2025

Then no idea... Sorry!

@vsarunas
Copy link
Contributor Author

vsarunas commented Oct 9, 2025

Since the pull request is coming from a fork of the repository, there is a need for an explicit permissions to edit someone else's branch.

It maybe that I need to enable Allow edits from maintainers while creating the PR; I'll look for this permission in the next PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FranzBusch FranzBusch FranzBusch left review comments

@weissi weissi weissi approved these changes

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vsarunas @weissi @hassila @FranzBusch