-
Notifications
You must be signed in to change notification settings - Fork 535
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
feat: Opsgenie plugin implementation #6154
Conversation
Amazing, @keon94 Would you like to help review this PR? thanks. |
Hey @klesh, thanks for the response! I also submitted this PRs to complement Opsgenie DOCs reference in config-ui. apache/incubator-devlake-website#649 Posting here because I think they are related |
return err | ||
} | ||
|
||
err = collectorWithState.InitCollector(api.ApiCollectorArgs{ |
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.
Does their API not support incremental collections? Meaning, to query based on incident creation time? Ideally, that's what we want otherwise we'll be pulling everything each time the blueprint runs.
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 feedback @keon94 ! So, I was testing the use for api.NewStatefulApiCollectorForFinalizableEntity. I just made a commit using this approach in the file backend/plugins/opsgenie/tasks/incidents_collector.go
. With that said, I could not implement CollectUnfinishedDetails to fetch individual incidents that have different status then "closed" or "resolved". When trying to use CollectUnfinishedDetails the raw table is created but never populated =(
It is mandatory to use CollectUnfinishedDetails if we want to query based on incident creation time?
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.
Just a additional information, Opsgenie API falls in the case of "Undetermind Strategy", because you can sort by Created Date in Descending order.
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.
CollectUnfinishedDetails is something that runs when you already have existing unresolved incidents from the previous run. The purpose of it is to grab those incidents from the DB and check them against the API and pull down their newest state. It is pretty important to implement this otherwise the collected incidents will remain "stuck" in whatever state they were initially collected with and never get updated.
Have you looked at the PagerDuty implementation of it as reference?
CollectUnfinishedDetails: &api.FinalizableApiCollectorDetailArgs{ |
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 @keon94 is proposing to use another helper
https://github.com/apache/incubator-devlake/blob/62a1e93a2dab358af6b0e2b1eecd21ad70208941/backend/plugins/pagerduty/tasks/incidents_collector.go#L69C24-L69C67
and I believe he is right unless the Opsgenie API supports filtering by updatedAt
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.
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.
Yep, that's expected behavior because it runs only if there's existing data in the table based on the output of the BuildInputIterator function.
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.
It appears to be working now heheh
InputRowType: reflect.TypeOf(models.User{}), | ||
Input: cursor, | ||
Convert: func(inputRow interface{}) ([]interface{}, errors.Error) { | ||
user := inputRow.(*models.User) |
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.
are teams and users necessary to collect for the purpose of Dora metrics? Or is this plugin doing more than that?
cc @klesh
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.
Hmm, you are right! In the context of DORA metrics I don't think they are necessary. My rational was to collect teams
and users
in order to populate the responders
table and be able to relate the issue with them and create the issue_assignee
table (because an Opsgenie Incident does not have an assignee for it, only responders
. It is a bit different from the concept of Incident in Pagerduty).
But, if you think it is overextending the needs for the plugin I'll be happy to remove them =)
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 see. In that case, it is ok. Let's keep this.
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 about config-ui
.
Query: func(reqData *api.RequestData, createdAfter *time.Time) (url.Values, errors.Error) { | ||
query := url.Values{} | ||
|
||
query.Set("query", fmt.Sprintf("impactedServices:%s", data.Options.ServiceId)) |
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.
You should be able to append the "createdAt" query to the query variable and remove the GetCreated function on L89. See the doc here:
https://support.atlassian.com/opsgenie/docs/search-syntax-for-incidents/
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.
According to the doc, I agree with @keon94 and we should be using the same collection helper
as pagerduty
plugin.
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.
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.
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.
Do you mean you need to be in an upgraded account to make that query? Is your token on a free plan?
What happens when you call the endpoint with this query right now? Anything in the response that says something like "query unsupported"?
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.
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.
Ok. Here's what I suggest then. Change the collection code back to how you had it first (full collection with no state), and put the state based logic in another branch. Then we'll merge this branch without that logic (please address the failing github checks first).
Go ahead and create another issue and call it "Support incremental collection for Opsengie plugin" and link that branch you created to it.
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.
We'll worry about the performance enhancement in that ticket
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 just pushed a change implementing the CollectUnfinishedDetails: &api.FinalizableApiCollectorDetailArgs{
and it is working properly. I just updated my branch and made a few changes to remote plugin api (adhering to the way Pagerduty plugin is doing like you oriented me).
That said, I used the GetCreated function.
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.
We had code-frozen for v0.20, this PR would be merged on v0.21, with an ETA of the end of this month. |
9dddf27
@klesh I've approved it. I think the plugin code looks ok. Please re-review and/or merge when you see fit. |
@keon94 @sandesvitor Nice work, folks, will look into it as soon as v0.20 is released |
@sandesvitor Seems like the config-ui failed, would you like to fix them as well? thanks. |
Thanks @klesh! Another question, could I commit ammend to fix one of my previous commit messages (that is breaking the lint)? I'm just a little bit confuse on how to write the command heheh Something like this: git commit --amend -m "fix(check): commit message" |
@sandesvitor i believe what you need is https://jacopretorius.net/2013/05/amend-multiple-commit-messages-with-git.html |
* fix: normalize_bp_settings throws error * fix: pg ci
@sandesvitor please help to resolve the lint errors on config-ui. |
…or-devlake into feat-opsgenie-plugin
Done @d4x1 ! =) |
Ah @klesh I forgot about the logo heheh the logo I used in config-ui is just a placeholder. It is to update in this PR or we have to wait? |
@sandesvitor No problem, you may update the PR |
Got it @klesh ! But there is a any way to get the Opsgenie logo in Devlake standart? (I'm not a designer hahha) |
@Startrekzky Please take a look. |
Hi @sandesvitor , I'll take care of the logo. I'll send it to you when it's done. |
Hi @sandesvitor , this is the logo in SVG format. Please check it out. |
Very nice @Startrekzky ! =) Works perfectly! |
Hi @klesh , since the v0.20 branch has been checked out, we can merge this PR now. Cc. @sandesvitor |
Hi @sandesvitor , I would like to nominate you as the Apache Committer. Your exceptional contributions to the Opsgenie plugin make you an ideal candidate for this. As an Apache Committer, you'll have the opportunity to make direct code contributions with push permission and collaborate closely with the community. If interested, please let me know, and I'll guide you through the process. You can DM me (@louis Zhang) in the Slack community as well. |
Summary
Adds a new plugin for Atlassian Opsgenie. Also includes Opsgenie Dashboard for Grafana.
Does this close any open issues?
Closes #6055
Screenshots
Include any relevant screenshots here.
Other Information
Any other information that is important to this PR.