search

haskell-github-trust / ekg-core

Library for tracking system metrics
BSD 3-Clause "New" or "Revised" License
40 stars 39 forks source link

Follow new naming in GHC.Stats? #29

Open nh2 opened 5 years ago

nh2 commented 5 years ago

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 https://github.com/ghc/ghc/commit/24e6594cc7890babe69b8ba122d171affabad2d1#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 commented 5 years ago

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)

https://github.com/tibbe/ekg-core/commit/a5cd48ca1c89d387a14482134fb3e4c3bd45bcaf#diff-51685f3c4893c24352025eb92fe6a11eR437-

This is likely because of commit https://github.com/ghc/ghc/commit/ef9e3483150ad759938dacebf47e5c484c451602 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 commented 5 years ago

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

nh2 commented 5 years ago

I'll make a PR for it.

nh2 commented 5 years ago

@23Skidoo Shall I base it on https://github.com/tibbe/ekg-core/pull/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 commented 5 years ago

PR at #30

tibbe commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

nh2 commented 5 years ago

(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.