-
Notifications
You must be signed in to change notification settings - Fork 214
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
Fix Jira Metrics Behavior #5360
Changes from all commits
55e5ecb
5dfefd3
7efbcae
4f619dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
import org.opensearch.dataprepper.model.plugin.PluginFactory; | ||
import org.opensearch.dataprepper.model.record.Record; | ||
import org.opensearch.dataprepper.model.source.Source; | ||
import org.opensearch.dataprepper.plugins.source.jira.rest.JiraRestClient; | ||
import org.opensearch.dataprepper.plugins.source.jira.rest.auth.JiraAuthConfig; | ||
import org.opensearch.dataprepper.plugins.source.jira.utils.JiraConfigHelper; | ||
import org.opensearch.dataprepper.plugins.source.source_crawler.CrawlerApplicationContextMarker; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
final JiraRestClient jiraRestClient) { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment for crawler initialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as jira service. Also crawler will be shared by multiple source plugins in the future |
||
jiraRestClient.setPluginMetrics(pluginMetrics); | ||
|
||
this.jiraSourceConfig = jiraSourceConfig; | ||
this.jiraOauthConfig = jiraOauthConfig; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,17 +55,20 @@ public class JiraRestClient { | |
private static final String ISSUES_REQUESTED = "issuesRequested"; | ||
private final RestTemplate restTemplate; | ||
private final JiraAuthConfig authConfig; | ||
private final Timer ticketFetchLatencyTimer; | ||
private final Timer searchCallLatencyTimer; | ||
private final Timer projectFetchLatencyTimer; | ||
private final Counter issuesRequestedCounter; | ||
private final PluginMetrics jiraPluginMetrics = PluginMetrics.fromNames("jiraRestClient", "aws"); | ||
private Timer ticketFetchLatencyTimer; | ||
private Timer searchCallLatencyTimer; | ||
private Timer projectFetchLatencyTimer; | ||
private Counter issuesRequestedCounter; | ||
private PluginMetrics jiraPluginMetrics; | ||
private int sleepTimeMultiplier = 1000; | ||
|
||
public JiraRestClient(RestTemplate restTemplate, JiraAuthConfig authConfig) { | ||
this.restTemplate = restTemplate; | ||
this.authConfig = authConfig; | ||
} | ||
|
||
public void setPluginMetrics(PluginMetrics pluginMetrics){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can pluginMetrics be passed in JiraRestClient constructor ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No for the same reason as Jira Service |
||
this.jiraPluginMetrics = pluginMetrics; | ||
ticketFetchLatencyTimer = jiraPluginMetrics.timer(TICKET_FETCH_LATENCY_TIMER); | ||
searchCallLatencyTimer = jiraPluginMetrics.timer(SEARCH_CALL_LATENCY_TIMER); | ||
projectFetchLatencyTimer = jiraPluginMetrics.timer(PROJECTS_FETCH_LATENCY_TIMER); | ||
|
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 pluginMetrics be added as part of JiraService constructor line 66 ?
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.
Tried that and it doesnt work because JiraService is a bean I think
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 we remove the the bean annotation for JiraService ?
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 that would cause a lot of problems? not sure tho have to ask @san81
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-plugins/saas-source-plugins/jira-source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/JiraService.java
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlvenable Yah, I like the approach to include
PluginMetrics
in the application context. We will try that approach and update here.