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.

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

Fix user resource usage metrics #876

Merged
merged 4 commits into from May 27, 2019
Merged

Fix user resource usage metrics #876

merged 4 commits into from May 27, 2019

Conversation

mprimi
Copy link
Contributor

@mprimi mprimi commented May 24, 2019

No description provided.

The previous implementation was making incorrect use of gauges, leading to inconsistent metric values.
More correctly model the underlying database by always having a value in the memory_used column.
@mprimi mprimi added this to the 4.0.0 milestone May 24, 2019
@mprimi mprimi requested a review from tgianos May 24, 2019 19:49
@mprimi mprimi self-assigned this May 24, 2019
@mprimi mprimi requested a review from ajoymajumdar May 24, 2019 19:49
@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9d7cb6b). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #876   +/-   ##
=========================================
  Coverage          ?   88.58%           
  Complexity        ?     3191           
=========================================
  Files             ?      394           
  Lines             ?    12813           
  Branches          ?      877           
=========================================
  Hits              ?    11350           
  Misses            ?     1074           
  Partials          ?      389
Impacted Files Coverage Δ Complexity Δ
...ecution/statemachine/actions/CleanupJobAction.java 96.77% <100%> (ø) 8 <4> (?)
...cution/statemachine/actions/HandleErrorAction.java 95.65% <100%> (ø) 6 <2> (?)
...execution/statemachine/actions/ShutdownAction.java 90.9% <100%> (ø) 7 <3> (?)
...cution/statemachine/listeners/LoggingListener.java 100% <100%> (ø) 25 <3> (?)
...ecution/statemachine/actions/InitializeAction.java 94.11% <100%> (ø) 4 <1> (?)
.../com/netflix/genie/agent/cli/GenieAgentRunner.java 100% <100%> (ø) 8 <0> (?)
...ecution/statemachine/actions/MonitorJobAction.java 100% <100%> (ø) 5 <1> (?)
...etflix/genie/web/tasks/leader/UserMetricsTask.java 100% <100%> (ø) 10 <7> (?)
...xecution/statemachine/actions/LaunchJobAction.java 100% <100%> (ø) 5 <1> (?)
...execution/statemachine/actions/SetUpJobAction.java 91.59% <100%> (ø) 36 <1> (?)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d7cb6b...2d74927. Read the comment docs.

@coveralls
Copy link

coveralls commented May 24, 2019

Coverage Status

Coverage decreased (-0.08%) to 91.54% when pulling 2d74927 on mprimi:master into 9d7cb6b on Netflix:master.

 - Remove low-level state machine transition messages
 - Add human readable messages for major changes of state during execution
@@ -93,7 +92,7 @@ public void eventNotAccepted(final Message<Events> event) {
*/
@Override
public void transition(final Transition<States, Events> transition) {
UserConsole.getLogger().info(
log.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional to not use UserConsole.getLogger()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was printing the agent state machine transition changes in the user terminal.
That's a bit too low level, so I'm just sending it to the log file instead.

this.activeUsersCount.set(summaries.size());

// Track users who previously had jobs but no longer do
final Set<String> usersToReset = Sets.newHashSet(this.userResourcesRecordMap.keySet());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use Sets.difference(this.userResourcesRecordMap.keySet(),summaries.keySet())

Copy link
Contributor Author

@mprimi mprimi May 24, 2019

Choose a reason for hiding this comment

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

There are a couple of ways, such as Sets.difference that do look better in terms of readability. However they produce views of the sets and then throw ConcurrentModificationException when I'm iterating this difference and removing from the map.

@mprimi mprimi merged commit 2258f0c into Netflix:master May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants