Inject MetricsPersistence directly into PersistingMetricsReporter#4708
Inject MetricsPersistence directly into PersistingMetricsReporter#4708bowsii wants to merge 2 commits into
Conversation
| @Produces | ||
| @RequestScoped | ||
| public MetricsPersistence metricsPersistence(CallContext callContext) { | ||
| return callContext.getPolarisCallContext().getMetricsPersistence(); |
There was a problem hiding this comment.
#4655 implies avoiding indirection via CallContext 😉
Would you mind taking this one step further and removing MetricsPersistence from CallContext? I do not think there is any reason for it to be there after this PR. This producer can deal with the factory directly (cf. polarisCallContext() on line 138).
The benefit would be that MetricsPersistence would be materialized only on API calls involving metrics reporting.
If you prefer that can be done as a separate follow-up PR. Current change is good and self-contained 👍
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
Thanks for the suggestion! I've taken it one step further and removed This means I've also updated the affected call sites and tests accordingly. |
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
I think we're making progress :) a few more minor comments and I think we can merge.
| @NonNull PolarisCallContext callCtx, @NonNull ScanMetricsRecord record) { | ||
| callCtx.getMetricsPersistence().writeScanReport(record); | ||
| BasePersistence metaStore = callCtx.getMetaStore(); | ||
| if (metaStore instanceof MetricsPersistence metricsPersistence) { |
There was a problem hiding this comment.
This code is no longer invoked, I think... why not remove it?
| @NonNull PolarisCallContext callCtx, @NonNull CommitMetricsRecord record) { | ||
| callCtx.getMetricsPersistence().writeCommitReport(record); | ||
| BasePersistence metaStore = callCtx.getMetaStore(); | ||
| if (metaStore instanceof MetricsPersistence metricsPersistence) { |
| @RequestScoped | ||
| public MetricsPersistence metricsPersistence( | ||
| RealmContext realmContext, MetaStoreManagerFactory metaStoreManagerFactory) { | ||
| BasePersistence metaStore = metaStoreManagerFactory.getOrCreateSession(realmContext); |
There was a problem hiding this comment.
Why bother calling getOrCreateSession() now? This producer will be invoked by CDI only for use cases that need a MetricsPersistence, not BasePersistence 🤔
The old optimization of reusing the same object is no longer relevant.
I think calling getOrCreateMetricsPersistence() (below) is sufficient.
|
FYI: @obelix74 |
2 participants
Fixes #4655
Added a CDI producer for MetricsPersistence in ServiceProducers.
Removed CallContext dependency from PersistingMetricsReporter.
Removed PolarisMetaStoreManager dependency from
PersistingMetricsReporter.
Injected MetricsPersistence directly into
PersistingMetricsReporter.
Replaced:
metaStoreManager.writeScanMetrics(...)
with:
metricsPersistence.writeScanReport(...)
Replaced:
metaStoreManager.writeCommitMetrics(...)
with:
metricsPersistence.writeCommitReport(...)
Updated Javadocs to reflect direct MetricsPersistence usage.
Refactored PersistingMetricsReporterTest:
writeScanReport() and writeCommitReport().
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)