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.

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

Follow new naming in GHC.Stats? #29

Open
nh2 opened this issue Nov 8, 2018 · 9 comments · May be fixed by #30
Open

Follow new naming in GHC.Stats? #29

nh2 opened this issue Nov 8, 2018 · 9 comments · May be fixed by #30

Comments

@nh2
Copy link

nh2 commented Nov 8, 2018

There is currently a naming mismatch between EKG and GHC.Stats.

For example, we have

     , ("rts.gc.current_bytes_used"       , Gauge . fromIntegral . Stats.gcdetails_live_bytes . Stats.gc)

so current_bytes_used = gcdetails_live_bytes.

This is due to the 2 years old GHC commit ghc/ghc@24e6594#diff-00007a115377c1a1c96e619128dcb206L236 which did this rename.

Should EKG eventually follow the new naming?

It's weird when some stats have the same names as their GHC.Stats counterparts, but others don't.
@nh2
Copy link
Author

nh2 commented Nov 8, 2018

There are also oddities like this, where two apparently very different concepts are filled in by the same variable:

     , ("rts.gc.par_tot_bytes_copied"     , Gauge . fromIntegral . Stats.par_copied_bytes)
     , ("rts.gc.par_avg_bytes_copied"     , Gauge . fromIntegral . Stats.par_copied_bytes)

a5cd48c#diff-51685f3c4893c24352025eb92fe6a11eR437-

This is likely because of commit ghc/ghc@ef9e348 which renamed it, but EKG also keeping the old (and as per the GHC commit, wrong and confusing) name seems like something we'd want to clean up enventually.

@23Skidoo
Copy link
Collaborator

23Skidoo commented Nov 8, 2018

Let's go for it (unless @tibbe objects). This will require a major version bump.

@nh2
Copy link
Author

nh2 commented Nov 8, 2018

I'll make a PR for it.

@nh2
Copy link
Author

nh2 commented Nov 8, 2018

@23Skidoo Shall I base it on #28 ?

Looks like we very very likely want to have that, and I can add in the newly added stats in the same run.

nh2 added a commit to nh2/ekg-core that referenced this issue Nov 8, 2018
@nh2
Follow new naming in GHC.Stats. Fixes haskell-github-trust#29.
05d5332 
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 (provided we're
on base >= 4.10; the #ifdef for even older base versions remains
unchanged).

This change may break some users that rely on the old names,
so a new major release should be made.
@nh2 nh2 linked a pull request Nov 8, 2018 that will close this issue
Open
@nh2
Copy link
Author

nh2 commented Nov 8, 2018

PR at #30

nh2 added a commit to nh2/ekg-core that referenced this issue Nov 8, 2018
@nh2
Follow new naming in GHC.Stats. Fixes haskell-github-trust#29.
caa5d68 
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 (provided we're
on base >= 4.10; the #ifdef for even older base versions remains
unchanged).

This change may break some users that rely on the old names,
so a new major release should be made.
@tibbe
Copy link
Collaborator

tibbe commented Nov 8, 2018

If long enough time has passed I think it makes sense to break backwards compatibility. Ideally we add the right name in release N and in some later release N+X we remove the old one. This way people don't have to throw ifdefs everywhere.

@nh2
Copy link
Author

nh2 commented Nov 8, 2018

@tibbe Not sure if you meant that, but since you brought in ifdefs, I may clarify:

The Haskell types backward compat isn't broken by this, the included #ifdefs take care of that.

It's the "dynamic" names of the fileds (e.g. rts.gc.current_bytes_used vs rts.gc.live_bytes) that may, at runtime, break those that rely on these names (for example the ekg UI's JS, which I'm fixing currently).

@nh2
Copy link
Author

nh2 commented Nov 8, 2018

(Don't merge yet, I noticed that the haddocks also have the old names to be updated, I still need to do that.)

nh2 added a commit to nh2/ekg-core that referenced this issue Nov 8, 2018
@nh2
Follow new naming in GHC.Stats. Fixes haskell-github-trust#29.
1da8410 
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 (provided we're
on base >= 4.10; the #ifdef for even older base versions remains
unchanged).

This change may break some users that rely on the old names,
so a new major release should be made.

The haddocks no longer explain the meaning of each field;
instead, GHC.Stats docs are linked now.
This is acceptable because the reworked GHC.Stats is much clearer
on meaning and units. It also means we cannot forget updating
ekg-core's haddocks to reflect GHC.Stats docs improvements.
@nh2
Copy link
Author

nh2 commented Nov 8, 2018

(Don't merge yet, I noticed that the haddocks also have the old names to be updated, I still need to do that.)

I have reworked the haddocks now, please see the amended commit.

nh2 added a commit to nh2/ekg-core that referenced this issue Mar 25, 2020
@nh2
Follow new naming in GHC.Stats. Fixes haskell-github-trust#29.
80bed30 
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.

The haddocks no longer explain the meaning of each field;
instead, GHC.Stats docs are linked now.
This is acceptable because the reworked GHC.Stats is much clearer
on meaning and units. It also means we cannot forget updating
ekg-core's haddocks to reflect GHC.Stats docs improvements.
nh2 added a commit to nh2/ekg-core that referenced this issue Mar 25, 2020
@nh2
Follow new naming in GHC.Stats. Fixes haskell-github-trust#29.
db6bfe4 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Follow new naming in GHC.Stats, add new stats nh2/ekg-core
3 participants
@tibbe @23Skidoo @nh2