Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Add TM GC CPU fraction to stat endpoint#3002

Merged
dneuman64 merged 2 commits into
apache:masterfrom
rob05c:tm-add-gc-cpu-stat
Nov 19, 2018
Merged

Add TM GC CPU fraction to stat endpoint#3002
dneuman64 merged 2 commits into
apache:masterfrom
rob05c:tm-add-gc-cpu-stat

Conversation

@rob05c
Copy link
Copy Markdown
Member

@rob05c rob05c commented Nov 7, 2018

What does this PR do?

Adds TM GC CPU fraction to stat endpoint, for debugging and profiling.

Which TC components are affected by this PR?

  • Documentation
  • Grove
  • Traffic Analytics
  • Traffic Monitor
  • Traffic Ops
  • Traffic Ops ORT
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Traffic Vault
  • Other _________

What is the best way to verify this PR?

Run Monitor, verify /publish/Stats endpoint has gc-cpu-fraction key.

Check all that apply

  • This PR includes tests
  • This PR includes documentation updates
  • This PR includes an update to CHANGELOG.md
  • This PR includes all required license headers
  • This PR includes a database migration (ensure that migration sequence is correct)
  • This PR fixes a serious security flaw. Read more: www.apache.org/security

@rob05c rob05c added new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor labels Nov 7, 2018
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 7, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2752/
Test PASSed.

Copy link
Copy Markdown
Member

@ezelkow1 ezelkow1 left a comment

Choose a reason for hiding this comment

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

Looks good, GCCPUfraction has been around since go1.5 and could be helpful to monitor usage

Copy link
Copy Markdown
Contributor

@dneuman64 dneuman64 left a comment

Choose a reason for hiding this comment

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

I know @ezelkow1 took a look at the code changes here, but I think this PR would benefit from a test or two.
It looks like there currently isn't any tests for this Stats struct (if there is, why didn't get get updated?), so I think our code would benefit from at least a test of the struct.

IMO Traffic Monitor is light on tests as is, and this seems like a good time to add some. It would definitely help future developers.

As required by PR Review.
@rob05c rob05c force-pushed the tm-add-gc-cpu-stat branch from ab0a1a7 to 7140940 Compare November 17, 2018 19:46
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 17, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2798/
Test FAILed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Nov 17, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2799/
Test FAILed.

@dneuman64
Copy link
Copy Markdown
Contributor

Thanks Rob! I know it can be a PITA do to tests, and it's frustrating when you have to write a test after changing one little thing. I am definitely not "unit test guy" but I think even a little test like this can make our products better and make the lives of future developers much easier. I appreciate you writing the test.

@dneuman64 dneuman64 merged commit db699c7 into apache:master Nov 19, 2018
@dneuman64 dneuman64 self-assigned this Nov 19, 2018
@rob05c rob05c deleted the tm-add-gc-cpu-stat branch January 2, 2019 20:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

new feature A new feature, capability or behavior Traffic Monitor related to Traffic Monitor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants