-
Notifications
You must be signed in to change notification settings - Fork 5
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
[DT-1074] Move to request scoped metrics #1889
[DT-1074] Move to request scoped metrics #1889
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.
Looks OK, just a few minor things
src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java
Outdated
Show resolved
Hide resolved
src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java
Outdated
Show resolved
Hide resolved
src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
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.
Can you add a PR description? I'm not exactly sure what is happening. Are you just removing the incorrect log information? Or does this provide a fix of that information?
@@ -11,38 +13,36 @@ | |||
* API request will get grouped together even if they are set in different methods. | |||
*/ | |||
@Component | |||
@RequestScope |
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.
🤔 I think request scope is the right approach for this component. However, this change means that this class can now only be used in a request context. Before, if it was referenced outside of a request context (such as in a flight), the data would be stored in a map but never used. Now it will generate a runtime exception.
There are only three uses of this currently and all are with a request scope, but it's something to keep in mind.
@rjohanek done |
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.
thanks for the description
Jira ticket: https://broadworkbench.atlassian.net/browse/dt-1074
Addresses
This adds request scoping to replace threads managing the cache. The advantage of this is the request scoping cleans things up for us, whereas if the thread is directly in control the cache may not be cleared. Making this change helps to ensure that data doesn't bleed from one request to another.
Summary of changes
Testing Strategy