Follow new naming in GHC.Stats, add new stats by nh2 · Pull Request #30 · haskell-github-trust/ekg-core

nh2 · 2018-11-08T14:43:25Z

Fixes #29.

Based on #28 for GHC 8.6 compatibility.

I've tested it with:

Stackage lts-12.17
Stackage nightly-2018-11-08

nh2 · 2018-11-08T14:48:33Z

Urgh, cumulative_par_balanced_copied_bytes has a missing @since 4.12.0.0 in https://downloads.haskell.org/~ghc/master/libraries/html/base/GHC-Stats.html, will fix.

nh2 · 2018-11-08T15:27:02Z

Looks like the ekg package's Javascript also has to be updated, it refers to the old names and there's no compile-time check for that:

https://github.com/tibbe/ekg/blob/07ac776ef90b8d6f5629568a5a28d911a0e267b0/assets/monitor.js#L337

That's why all my graphs are empty if just applying this ekg-core patch, and all values in the top right are 0.

See haskell-github-trust/ekg-core#30

23Skidoo · 2018-11-08T20:45:50Z

I think I'll do a GHC 8.6-compatible point release first, and then a major release with this change. ekg-statsd, ekg-json, and ekg may also require updates.

23Skidoo · 2018-11-08T20:54:41Z

System/Metrics.hs

     ])
    getRTSStats
 #else
+    -- Pre base-4.10 we have the names from before GHC commit 24e6594cc7890babe69b8ba122d171affabad2d1.


Doesn't seem like a great idea to me. Can't we use new names with base <4.10 as well?

Maybe, but then we'd have to put a new ifdef for that very version in which the old names were in place.

I didn't see the value in updating the EKG names for significantly older versions of base/ghc, but if you think there is one, I can try and do that.

Our GHC support window is 3 years, otherwise I'd gladly drop support for base < 4.10.

23Skidoo · 2018-11-08T20:58:03Z

System/Metrics.hs

-- @par_tot_bytes_copied@ divided by @par_max_bytes_copied@ approaches
-- 1 for a maximally sequential run and approaches the number of
-- threads (set by the RTS flag @-N@) for a maximally parallel run.
+-- Registered counters (see "GHC.Stats" for their meanings:


I'd rather leave the old comment (adding "see also the GHC.Stats documentation" is fine).

The old comment would have to be fully updated with the new fields, and that would be mostly copy-pasting stuff from GHC.Stats, so I'm not sure how useful that would be. In particular because that copy-paste easily gets outdated (like the thing in #29 (comment)).

The old comment was also basically copy-paste. Since ekg tries to provide a uniform interface over a range of GHC versions, I think it's useful to have a comment describing the ekg view of the world. If the comment gets out of date due to changes in GHC, it means that our assumptions have changed and we may need to update other parts of library.

The issue in #29 (comment) appears to be caused by us trying to provide backwards compatibility.

OK, done.

I've copied the new descriptions from https://hackage.haskell.org/package/base-4.12.0.0/docs/GHC-Stats.html.

23Skidoo · 2018-11-08T21:01:33Z

System/Metrics.hs


 #if MIN_VERSION_base(4,10,0)
-- | Convert nanoseconds to milliseconds.
-nsToMs :: Int64 -> Int64


Can this break anything?

I don't think so, as I removed all usages of this function.

I was rather thinking of clients breaking due to ms -> ns change. We should mention it in the changelog.

Yes, the fields and their meanings definitely change completely.

nh2 · 2018-11-09T14:26:39Z

I think I'll do a GHC 8.6-compatible point release first, and then a major release with this change.

That sounds prudent to me.

ekg-statsd, ekg-json, and ekg may also require updates.

ekg for sure, I already have a branch that does this which I'm currently testing. The other two I'm not using so I might need some help with those.

23Skidoo · 2018-11-19T20:40:27Z

I think I'll do a GHC 8.6-compatible point release first

This is now done.

For easier GHC compatibility testing.

GHC commit 24e6594cc7890babe69b8ba122d171affabad2d1 changed a lot of the stats names. Until now, EKG stuck to the old names, which can be very confusing; especially since some names were clearly misleading and have been renamed in GHC consequentially. This commit changes all names to the new names. For base < 4.10, we translate the old API to their new equivalents (multiplying to get nanoseconds as needed). The added comment documents this translation. The change has been tested on Stackage versions: * lts-14.27 (GHC 8.6.5) * nightly-2018-11-08 (GHC 8.6.1) * lts-12.17 (GHC 8.4.4) * lts-9.21 (ghc-8.0.2) -- this is base 4.9 This change may break some users that rely on the old names, so a new major release should be made.

nh2 · 2020-03-25T01:04:26Z

Doesn't seem like a great idea to me. Can't we use new names with base <4.10 as well?

I have force-pushed to implement this suggestion (and also to address all other feedback). Now it translates the old API to the new field names.

For easy review comparison I have diffed the fields used by not(MIN_VERSION_base(4,10,0)) and MIN_VERSION_base(4,10,0) in meld, showing that the first is a subset of the other:

The change has now been tested on Stackage versions:

lts-14.27 (GHC 8.6.5)
nightly-2018-11-08 (GHC 8.6.1)
lts-12.17 (GHC 8.4.4)
lts-9.21 (ghc-8.0.2) -- this is base 4.9

nh2

Answered all feedback.

nh2 · 2020-03-25T01:09:05Z

ekg javascript/HTML PR at haskell-github-trust/ekg#80

nh2 · 2020-03-25T01:13:47Z

ekg-statsd, ekg-json, and ekg may also require updates.

ekg: Update to new ekg-core stats names ekg#80 (only needs a few fields renamed)
ekg-json: seems to require no update, as it does not refer to any specific field names
ekg-statsd: same

eborden · 2021-11-10T22:14:22Z

This PR seems worth resurrecting. There are now additional stats from base-4.14.1.0+ tracking the non moving GC. What is pending to get this across the line. How can I help?

eborden · 2021-11-10T22:18:24Z

The newer stats are

Stats.RTSStats
  { nonmoving_gc_sync_cpu_ns
  , nonmoving_gc_sync_elapsed_ns
  , nonmoving_gc_sync_max_elapsed_ns
  , nonmoving_gc_cpu_ns
  , nonmoving_gc_elapsed_ns
  , nonmoving_gc_max_elapsed_ns
  }

and

Stats.GCDetails
  { gcdetails_nonmoving_gc_sync_cpu_ns
  , gcdetails_nonmoving_gc_sync_elapsed_ns
  }

NorfairKing · 2022-12-04T23:13:30Z

Gentle ping, another year later

nh2 mentioned this pull request Nov 8, 2018

Follow new naming in GHC.Stats? #29

Open

nh2 force-pushed the issue-29-new-ghc-stats-names-on-pr-28 branch from 05d5332 to caa5d68 Compare November 8, 2018 14:49

nh2 changed the title ~~Follow new naming in GHC.Stats~~ Follow new naming in GHC.Stats, add new stats Nov 8, 2018

nh2 force-pushed the issue-29-new-ghc-stats-names-on-pr-28 branch from caa5d68 to 1da8410 Compare November 8, 2018 16:00

nh2 added a commit to nh2/ekg that referenced this pull request Nov 8, 2018

Update to new ekg-core stats names.

f17787b

See haskell-github-trust/ekg-core#30

23Skidoo reviewed Nov 8, 2018

View reviewed changes

Add stack.yaml.

f3e7d15

For easier GHC compatibility testing.

nh2 force-pushed the issue-29-new-ghc-stats-names-on-pr-28 branch from 1da8410 to 80bed30 Compare March 25, 2020 00:44

nh2 force-pushed the issue-29-new-ghc-stats-names-on-pr-28 branch from 80bed30 to db6bfe4 Compare March 25, 2020 01:03

nh2 commented Mar 25, 2020

View reviewed changes

nh2 mentioned this pull request Mar 25, 2020

Update to new ekg-core stats names haskell-github-trust/ekg#80

Open

Conversation

nh2 commented Nov 8, 2018

Uh oh!

nh2 commented Nov 8, 2018

Uh oh!

nh2 commented Nov 8, 2018

Uh oh!

23Skidoo commented Nov 8, 2018

Uh oh!

23Skidoo Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

23Skidoo Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nh2 commented Nov 9, 2018

Uh oh!

23Skidoo commented Nov 19, 2018

Uh oh!

nh2 commented Mar 25, 2020

Uh oh!

nh2 left a comment

Choose a reason for hiding this comment

Uh oh!

nh2 commented Mar 25, 2020

Uh oh!

nh2 commented Mar 25, 2020

Uh oh!

eborden commented Nov 10, 2021

Uh oh!

eborden commented Nov 10, 2021

Uh oh!

NorfairKing commented Dec 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

23Skidoo Nov 8, 2018 •

edited

Loading

23Skidoo Nov 8, 2018 •

edited

Loading