-
Notifications
You must be signed in to change notification settings - Fork 569
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
Improve some interceptor code paths in metrics #3251
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This should help but we need to verify this with some benchmark numbers.
I did some local benchmarking before, during, and after I made these changes. Locally I see distinct improvement, but the results varied enough from run to run on my system that I have asked the PSR team to rerun their own metrics benchmark in their environment--using the revised code--to assess the improvement. |
The PSR team reran the metrics workload with the changes. The overall throughput has improved but is not all the way back to the baseline of 2.2.2. PSR helpfully provided the JRF file from the run, as with the original JRF from 2.3.1. The 2.3.1 JRF showed at least two distinct metrics-related hotspots, both of which the coding changes in this PR addressed. The JRF from my own local testing as well as from PSR show those hotspots are gone and I see no other metrics-related hotspots. It's plausible that other, non-metrics performance regressions have crept in. Given that the metrics workload includes processing HTTP requests (and responses) to trigger the metrics updates in the server the metrics workload could expose a regression along that code path as well. |
…deleted metrics themselves to avoid hash look-up of metric ID to check if interceptor refers to deleted metric; do a bit of optimization in computing the exemplar label(s) when exemplars are not actually in use
…y expensive lambda for FINEST logging; save and look-up which parameter position an async parameter occupies (if there is one) in a JAX-RS endpoint
…once, now that we use Lists (for in-request performance) instead of Sets
…mpty (meaning exemplars are not turned on)
…yDecaryingReservoir, because the value will change only every second
…ics to stop the scheduled updates to 'current-time-in-minutes' values in ExponentiallyDecayingReservoir instances
LOGGER.log(Level.WARNING, "Shutdown of current time updater timed out; continuing"); | ||
} | ||
} catch (InterruptedException e) { | ||
LOGGER.log(Level.WARNING, "InterruptedException caught while stopping the current time updater; continuing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: if you "eat" an InterruptedException
you should do Thread.currentThread().interrupt()
to preserve its interrupted status. Obviously here stuff is shutting down so it probably doesn't matter.
Optimize some metrics interceptor code paths.
The various commit messages describe the improvements.