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

Vim performance state time interval #22606

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 8, 2023

Moved Helper#get_time_interval to the concept that it represented.

second commit: it also was retrieving a hardcoded value, so avoided the lookup

@kbrock kbrock requested a review from agrare as a code owner July 8, 2023 00:21
self.capture_interval = 3600
self.capture_interval = 1.hour.to_i
Copy link
Member

Choose a reason for hiding this comment

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

If it's hardcoded, why even store it?

Copy link
Member

Choose a reason for hiding this comment

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

Oh it's a column? - Wild we have this at all.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with this for now - future feature just remove the column.

# @param obj object holding vps records
# @param timestamp [Time|String] hourly timestamp, prefer Time
# @returns [Range] time range
def self.get_time_interval(obj, timestamp)
Copy link
Member

Choose a reason for hiding this comment

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

obj isn't even used now - can we remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is only called for hourly.
I think it was written to support all VimPerformanceState.
But we only store vim performance state values for hours so it is probably moot.

changing

this is returning the time range for a vps, moved it to that code
VPS use a hard coded 1.hour range for the hourly captures.
This avoids looking up a record just to come up with the same conclusion
VimPErformanceStates are only stored for hourly.
And this is only called for hourly values

So passing the object is not needed.

If we change that and need to pass the object in the future, I'm sorry
@kbrock kbrock force-pushed the vim_performance_state_time_interval branch from 02a3f01 to 134ca05 Compare July 14, 2023 23:51
@kbrock
Copy link
Member Author

kbrock commented Jul 15, 2023

update:

  • rebased
  • dropped obj passed into vps_capture_interval (it is always going to be the same value)
  • changed name of variable from capture_interval to date_range.

@miq-bot
Copy link
Member

miq-bot commented Jul 15, 2023

Checked commits kbrock/manageiq@610473d~...cd86162 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 👍

@Fryguy Fryguy self-assigned this Jul 19, 2023
@Fryguy Fryguy merged commit c6bfffb into ManageIQ:master Aug 1, 2023
@kbrock kbrock deleted the vim_performance_state_time_interval branch August 1, 2023 22:44
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.

3 participants