Skip to content

Add alerting support #630

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

Closed
wants to merge 10 commits into from
Closed

Add alerting support #630

wants to merge 10 commits into from

Conversation

pcolladosoto
Copy link

@pcolladosoto pcolladosoto commented Sep 4, 2024

Hi @simPod! As seen on #34 there was some interest in having alerting support for the plugin.

I'm currently working on developing an OSS plugin for MongoDB databases and decided to base it off GrafanaJsonDatasource given how neat and to the point the codebase is.

In an effort to make alerting available for the soon-to-be MongoDB plugin I decided to add a full-fledged backend support to the GrafanaJsonDatasource plugin. Given how helpful it's been, the least I could do was to add this backend support before the code bases diverged so that you could integrate it into the plugin if you wanted to.

Anyhow, this PR strives to add a Go-based backend to the plugin so that it can leverage Grafana built-in goodies such as alerting. If I'm honest, the Infinity datasource plugin could be a bit overkill and, what's more, it doesn't add support for interactively building the query AFAIK.

Please bear in mind that the PR's contents are (very) rough around the edges. I just wanted to share the work before trying to transition it to something more robust to make sure you were interested. Otherwise I can just begin swapping out the HTTP-based backend for a MongoDB-based one and call it a day.

Also, please note that I have absolutely no idea about JS and the accompanying development environment. I've tried to play with it as little as possible but I do have some doubts; especially about how the ad-hoc filters can be applied.

Anyway, sorry for all the rambling and thanks for building GrafanaJsonDatasource and making it public 😄

P.D: Bear in mind some changes such as the authorship and so on have been altered thinking about the MongoDB plugin. All these changes can be revisited should a merge take place.

@simPod
Copy link
Owner

simPod commented Sep 4, 2024

Hello @pcolladosoto, I'm currently on currently on kitesurfing holiday so I'll review this the next week when I get back home. I don't consider myself a Go developer but definitely can take a look. Thank you for your contribution in advance.

@pcolladosoto
Copy link
Author

No worries! Have a great one! 🪁 🏄🏾

Copy link
Owner

@simPod simPod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did no go through Go code yet

@@ -0,0 +1,65 @@
import pathlib, sys, json, time
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this python server?

There's already on in https://github.com/simPod/GrafanaJsonDatasource/tree/0.7.x/examples/server

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really yeah... I figured I'd rather write my own than leverage the existing server's implementation because in the long(ish) run it'd differ as I implemented MongoDB-specific methods. However, it can be deleted for sure!

"name": "JSON",
"id": "simpod-json-datasource",
"name": "GTOM",
"id": "gtom",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done!

// https://grafana.com/developers/plugin-tools/how-to-guides/data-source-plugins/add-resource-handler
// https://grafana.com/developers/plugin-tools/tutorials/convert-a-frontend-datasource-to-backend#frontend-datasource-class

// const response = await this.getResource('metrics').catch((err) => {console.log(`error when getting metrics: ${JSON.stringify(err)}`)});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this need some cleanup?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it did... those comments are now gone!

@@ -60,7 +59,7 @@ export class DataSource extends DataSourceApi<GrafanaQuery, GenericOptions> {
return !query.hide;
}

query(options: QueryRequest): Promise<DataQueryResponse> {
oldQuery(options: QueryRequest): Promise<DataQueryResponse> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be ever called?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really: deleted!

@@ -0,0 +1,92 @@
module github.com/pcolladosoto/gtom-native

go 1.21
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
go 1.21
go 1.23

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already bumped up the version and tweaked the module names to target your repo instead of my fork :P

@pcolladosoto
Copy link
Author

Hi @simPod! Thanks a ton for the comments and the change requests! I'll be a bit busy this week, but I didn't want to go without saying that all your suggestions are on point. Given I used this datasource plugin as a stepping stone towards a MongoDB datasource I just opened the PR before the codebase began diverging a bit more. I just wanted to make sure you were interested in the PR before honing down on the different rough edges. I'll be sure to address what you brought up plus all the other TODOs I littered here and there... Thanks again!

@KrisztianKaszas
Copy link

Dear @simPod, as mentioned in #34 we also planned to do the same. With a year delay, we also started in August to build a grafana backend plugin around the data source plugin, but due to some personal bottlenecks we did not get so far as @pcolladosoto yet.

Do I understand right, the plan is to add @pcolladosoto solution to 0.7.x so we can put our efforts to achieve the same task on hold?

Is there an ETA when 0.7.x will got this feature? Can we assist with this in any way?

@pcolladosoto
Copy link
Author

Hi @KrisztianKaszas! My idea is to try and address the comments next week. However, I would really appreciate it if a second pair of eyes had a look at the changes: I'm sure there are many things to be improved!

Hopefully by the end of next week this PR can move into a review stage where all the changes have been migrated, hopefully making the inclusion in the plugin a reality soon.

Thanks a ton for your comment!

@KrisztianKaszas
Copy link

Hi @pcolladosoto, we will do a PR as well and will try to run some tests with our existing Grafana solution, that is using this great data source. If we find anything, will address them. BR!

Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Oct 26, 2024
Copy link

github-actions bot commented Nov 2, 2024

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Nov 2, 2024
@KrisztianKaszas
Copy link

Our developer has finished testing this request in November 2024 and did not found any issues. However it seems that his comment lost.

What are the plans with this feature?

@simPod
Copy link
Owner

simPod commented Jan 16, 2025

It's still free to be implemented.

@pcolladosoto
Copy link
Author

Hi @simPod and @KrisztianKaszas! Your comments triggered an email that reached my inbox and brought this back to my attention. Sorry about that...

I'll try to address the comments (for real this time :P) shortly. Sorry again for the delay...

@pcolladosoto
Copy link
Author

Hi @simPod! I've pushed most of the requested changes to my fork (i.e. pcolladosoto/gtom-native). I've also rebased everything on top of the current branch so as to avoid any possible merge conflicts.

However, it seems that I cannot reopen the PR after the GitHub bot closed it up... Could you please reopen it and/or open a new one? I don't want to mess things up too much...

At any rate, if you could have a look at the changes we can continue to move things along.

Sorry for the delay in getting to this...

Have a great one!

@KrisztianKaszas
Copy link

Hi @simPod! it seems the pull request was closed again with unmerged commits. Is there a way to bring this on back on track? Have a nice day!

@simPod
Copy link
Owner

simPod commented Mar 11, 2025

Sorry all, I'm kind of stuck as I've not upgraded to Grafana v11 yet so I had no time for this either.

@simPod
Copy link
Owner

simPod commented Mar 11, 2025

@pcolladosoto can you recreate PR? I'm unable to reopen this

image

@KrisztianKaszas
Copy link

@pcolladosoto / @simPod is there anything we can do? Since we have tested these commits, we could try to create a new ticket with the commits as well?

@simPod
Copy link
Owner

simPod commented Mar 31, 2025

Let's recreate the PR for start

@pcolladosoto
Copy link
Author

pcolladosoto commented Mar 31, 2025

Hi @simPod and @KrisztianKaszas! I just opened #689 to try and push the changes through. I'll do my best to reply as quickly as possible!

Please be sure to let me know about anything that might be missing.

Let's see if we can add this once and for all...

Thanks for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants