Fix Jira Metrics Behavior #5360
Conversation
…man (fix jira metrics behavior) Signed-off-by: Maxwell Brown <[email protected]>
Galactus22625
requested review from
sb2k16,
chenqi0805,
engechas,
san81,
srikanthjg,
graytaylor0,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
Signed-off-by: Maxwell Brown <[email protected]>
Signed-off-by: Maxwell Brown <[email protected]>
Signed-off-by: Maxwell Brown <[email protected]>
|
|
||
| public JiraService(JiraSourceConfig jiraSourceConfig, JiraRestClient jiraRestClient) { | ||
| this.jiraSourceConfig = jiraSourceConfig; | ||
| this.jiraRestClient = jiraRestClient; | ||
| } | ||
|
|
||
| public void setPluginMetrics(PluginMetrics pluginMetrics){ |
There was a problem hiding this comment.
Can pluginMetrics be added as part of JiraService constructor line 66 ?
There was a problem hiding this comment.
Tried that and it doesnt work because JiraService is a bean I think
There was a problem hiding this comment.
Can we remove the the bean annotation for JiraService ?
There was a problem hiding this comment.
I think that would cause a lot of problems? not sure tho have to ask @san81
There was a problem hiding this comment.
@dinujoh we are using Spring dependency injection here. We are not instantiating JiraService ourself. Spring DI is instantiating it and for Spring DI, any constructor parameter should also be a spring bean otherwise, it won't inject as the constructor argument. If PluginMetrics is a spring bean then constructor argument will work but it is not.
There was a problem hiding this comment.
Does this needs to use Spring dependency injection ? I was recommending to create the instance when needed in the client for example in JiraSource without using Spring DI
There was a problem hiding this comment.
Why can't we include PluginMetrics in the context so that we can provide it via the application context? That would be a good long-term solution.
There was a problem hiding this comment.
The idea is that, as we add additional SAAS sources, the common code in source-crawler will dynamically bind with one of the implementation in a specific SAAS source like Jira or Confluence etc. That is why we started using Spring DI in these source plugins.
There was a problem hiding this comment.
@dlvenable Yah, I like the approach to include PluginMetrics in the application context. We will try that approach and update here.
| @@ -54,9 +55,15 @@ public JiraSource(final PluginMetrics pluginMetrics, | |||
| final PluginFactory pluginFactory, | |||
| final AcknowledgementSetManager acknowledgementSetManager, | |||
| Crawler crawler, | |||
| PluginExecutorServiceProvider executorServiceProvider) { | |||
| PluginExecutorServiceProvider executorServiceProvider, | |||
| final JiraService jiraService, | |||
There was a problem hiding this comment.
How is this injected here ? Can JiraService be initialized in the JiraSource constructor ? and pass pluginMetrics to the JiraService constructor ?
There was a problem hiding this comment.
tried passing in pluginMetrics to jiraService constructor but doesn't work because jira service is a bean i think. Jira Service can be initialized in jiraSource constructor for the same reason
| super(PLUGIN_NAME, pluginMetrics, jiraSourceConfig, pluginFactory, acknowledgementSetManager, crawler, executorServiceProvider); | ||
| log.info("Creating Jira Source Plugin"); | ||
| jiraService.setPluginMetrics(pluginMetrics); | ||
| crawler.setPluginMetrics(pluginMetrics); |
There was a problem hiding this comment.
Same comment for crawler initialization.
There was a problem hiding this comment.
same as jira service. Also crawler will be shared by multiple source plugins in the future
|
|
||
| public void setPluginMetrics(PluginMetrics pluginMetrics){ |
There was a problem hiding this comment.
Can pluginMetrics be passed in JiraRestClient constructor ?
There was a problem hiding this comment.
No for the same reason as Jira Service
|
|
||
| public JiraService(JiraSourceConfig jiraSourceConfig, JiraRestClient jiraRestClient) { | ||
| this.jiraSourceConfig = jiraSourceConfig; | ||
| this.jiraRestClient = jiraRestClient; | ||
| } | ||
|
|
||
| public void setPluginMetrics(PluginMetrics pluginMetrics){ |
There was a problem hiding this comment.
Why can't we include PluginMetrics in the context so that we can provide it via the application context? That would be a good long-term solution.
Description
Jira Metrics were showing up under aws.jira and aws.jiraRestClient and crawer.sourceCrawler instead of
jira.pipelineName.
Fixed this issue
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.