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

Add reporting session threshold options in settings #17334

Merged
merged 1 commit into from
Apr 27, 2018
Merged

Add reporting session threshold options in settings #17334

merged 1 commit into from
Apr 27, 2018

Conversation

robbmanes
Copy link
Contributor

@robbmanes robbmanes commented Apr 23, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1572350

Adds reporting session thresholds to be used in a PR against
manageiq-ui-classic.

This PR is required for an incoming PR that replaces hard-coded settings in the reporting session with configurable options. Link will be provided once that PR is available.

@robbmanes
Copy link
Contributor Author

PR against the Classic UI that requires this setting is ManageIQ/manageiq-ui-classic#3831

@@ -989,6 +989,8 @@
:keep_reports: 6.months
:purge_window_size: 100
:queue_timeout: 1.hour
:session_log_threshold: 100.kilobytes
:session_element_threshold: 10.kilobytes
Copy link
Member

Choose a reason for hiding this comment

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

I think this should go in the session section (not reporting), below, then we can remove the session_ prefix:

(line 1046 below)

 :session:
   :element_threshold: 10.kilobytes
   :interval: 60
   :log_threshold: 100.kilobytes
   :memcache_server: 127.0.0.1:11211
   :memcache_server_opts: "-l 127.0.0.1 -I 1M"
   :show_login_info: true
   :timeout: 3600

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

See suggestion about moving this to the session section ^

@robbmanes
Copy link
Contributor Author

Should now be accessible via Settings.session.log_threshold and Settings.session.element_threshold.

@robbmanes
Copy link
Contributor Author

Also, just a reminder, these changes affect currently only report-generating sessions, at least right now. If the plan is to have this be able to be used in the future by anyone calling this for session data, then this makes sense, but I wanted to bring it up since currently :session seems to apply mainly to our memcached config.

@jrafanie
Copy link
Member

Also, just a reminder, these changes affect currently only report-generating sessions, at least right now. If the plan is to have this be able to be used in the future by anyone calling this for session data, then this makes sense, but I wanted to bring it up since currently :session seems to apply mainly to our memcached config.

I believe get_data_size is a generic recursive method that walks the session objects and logs when it exceeds these thresholds. I don't think your change here or in ui classic is limited to only reporting. It might just be that reporting is the most common offender for dumping large objects into the session.

I'm good with this change. I'll ping @mzazrivec to give his approval before merging.

@robbmanes
Copy link
Contributor Author

I believe get_data_size is a generic recursive method that walks the session objects and logs when it exceeds these thresholds. I don't think your change here or in ui classic is limited to only reporting.

Ah, I did not know that; I was using it explicitly to raise/alter limits in the classic UI since reporting was my offender in this case, but yes, anyone who calls get_data_size generically will be affected. Thanks!

@jrafanie
Copy link
Member

@robbmanes is there a BZ for this? Are we planning to backport this?

@robbmanes
Copy link
Contributor Author

It is for a specific use case, but I did not make a BZ - I just made one now here in bz1572350. They adjusted their monitoring utilities to ignore the WARN generated when they do reports but would prefer if this was configurable.

@jrafanie
Copy link
Member

@robbmanes can you git commit --amend and put "Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1572350" in the commit message (usually the 3rd line), then force push to your branch? The bot should pick up the link and automatically update the BZ.

Also, if this needs to be backported, please let me know what upstream branches (gaprindashvili, fine, etc.) so we can tag it. Note, both this change and UI classic change would need to be backported. Upstream branches to downstream versions can be found here: https://github.com/chrisarcand/miqversions

Adds reporting session thresholds to be used in a PR against
manageiq-ui-classic.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1572350
@robbmanes
Copy link
Contributor Author

Sure thing; commits amended in both repos. Concerning backport, I'll find out right now, I am pretty sure the interested party is running fine currently.

@miq-bot
Copy link
Member

miq-bot commented Apr 26, 2018

Checked commit https://github.com/robbmanes/manageiq/commit/4ebb1627a32474fe4136ba0d865f85ffcee49bc8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@robbmanes
Copy link
Contributor Author

Backports for both gaprindashvili and fine please, the party in question intends to upgrade in the coming months. Thanks!

@jrafanie
Copy link
Member

@miq-bot add_label gaprindashvili/yes
@miq-bot add_label fine/yes

@jrafanie jrafanie added this to the Sprint 85 Ending May 7, 2018 milestone Apr 27, 2018
@jrafanie jrafanie merged commit 37d782f into ManageIQ:master Apr 27, 2018
@jrafanie jrafanie self-assigned this Apr 27, 2018
@jrafanie
Copy link
Member

@robbmanes ok, merged :shipit: , now just update your UI classic PR description to include that BZ link like I put here (I have edit access here but not there)

"Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1572350"

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