Add alerting support #689
Add alerting support #689
Conversation
Signed-off-by: Pablo Collado Soto <[email protected]>
The original implementation already provides a Go-based server with a much better and more robust implementation.
There was a problem hiding this comment.
Pull Request Overview
This PR primarily updates API endpoints and refactors the data source classes to support alerting functionality while aligning with a new backend structure.
- Updated REST endpoint URLs to use the UID-based format.
- Modified the DataSource to extend DataSourceWithBackend and removed obsolete query and test functions.
- Added Go implementations for the datasource plugin and associated settings, along with basic tests.
Reviewed Changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/MetricFindQuery.ts | Updated URL endpoint for variable queries to the UID-based path. |
| src/DataSource.ts | Changed base class, updated resource endpoints, and removed obsolete methods. |
| pkg/plugin/datasource_test.go | Added a basic test for QueryData functionality. |
| pkg/plugin/datasource.go | Implemented a new datasource plugin with resource routing and error handling. |
| pkg/models/settings.go | Introduced PluginSettings with secret handling for API keys. |
| pkg/main.go | Configured the main entry point to use the new datasource. |
| Magefile.go | Added Magefile for build management using Grafana’s build SDK. |
Files not reviewed (3)
- Makefile: Language not supported
- go.mod: Language not supported
- src/plugin.json: Language not supported
Comments suppressed due to low confidence (2)
pkg/models/settings.go:26
- [nitpick] This log statement uses an error level for routine settings loading. Consider using a lower severity (like debug or info) to prevent misleading error reports.
backend.Logger.Error("loading settings", "source", source)
pkg/plugin/datasource.go:46
- [nitpick] Consider renaming 'cli' to 'httpClient' for improved clarity and consistency with other parts of the code.
cli, err := httpclient.New(opts)
| "metrics": true, | ||
| "logs": true, | ||
| "info": { | ||
| "author": { | ||
| "name": "Šimon Podlipský" | ||
| "name": "Šimon Podlipskýo" |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes introduce new build scripts and module configuration along with a complete implementation of a Grafana datasource plugin. Updates include modifications in the ignore settings and metadata configuration. New files provide build targets via Mage and Make, while the module dependencies are set in go.mod. The plugin implementation now supports data queries, health checks, and resource management, with corresponding tests added. Additionally, the frontend data source has been refactored to use updated API endpoints and backend functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Grafana
participant Main (pkg/main.go)
participant DS (pkg/plugin/datasource.go)
participant HTTP (External Service)
Grafana->>Main: Initialize Plugin
Main->>DS: Call datasource.Manage (NewDatasource)
DS->>HTTP: Send Query/Health Check Request
HTTP-->>DS: Return Response
DS-->>Main: Forward Data/Status
Main-->>Grafana: Deliver Response
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/plugin.json (1)
14-14: Revert author's name to address past feedback.
Previous reviewer comment ("revert") suggests reverting to the original name "Šimon Podlipský."
🧹 Nitpick comments (10)
go.mod (1)
7-92: Consider using Go modules tooling to manage indirect dependenciesThe file contains a large number of manually specified indirect dependencies. While comprehensive, this approach can be error-prone and difficult to maintain.
Consider using
go mod tidyto automatically manage indirect dependencies instead of manually listing them. This command will add any missing dependencies and remove unnecessary ones.go mod tidypkg/plugin/datasource_test.go (1)
25-27: Improve test failure messageThe current failure message doesn't specify what exactly is wrong with the responses.
Provide more context in the failure message to make debugging easier:
if len(resp.Responses) != 1 { - t.Fatal("QueryData must return a response") + t.Fatalf("Expected exactly 1 response, got %d", len(resp.Responses)) }pkg/main.go (1)
12-19: Update boilerplate comments to reference the correct datasource name.The comments reference "SampleDatasource" which appears to be boilerplate that wasn't updated for this implementation.
- // to exit by itself using os.Exit. Manage automatically manages life cycle - // of datasource instances. It accepts datasource instance factory as first - // argument. This factory will be automatically called on incoming request - // from Grafana to create different instances of SampleDatasource (per datasource - // ID). When datasource configuration changed Dispose method will be called and - // new datasource instance created using NewSampleDatasource factory. + // to exit by itself using os.Exit. Manage automatically manages life cycle + // of datasource instances. It accepts datasource instance factory as first + // argument. This factory will be automatically called on incoming request + // from Grafana to create different instances of the JSON datasource (per datasource + // ID). When datasource configuration changes, the Dispose method will be called and + // new datasource instance created using the NewDatasource factory.Makefile (3)
18-19: Backend build is Linux-only.The current build configuration only targets Linux. Consider adding multi-platform build support for broader compatibility.
backend: - mage -v build:linux + mage -v build:linux + # Uncomment the following lines to enable multi-platform builds + # mage -v build:windows + # mage -v build:darwin
15-16: Add dependency check for build tools.The build process assumes yarn is installed without verifying its presence.
frontend: + @which yarn > /dev/null || (echo "yarn not found. Please install it first." && exit 1) yarn build
18-19: Add dependency check for mage.Similar to yarn, the build assumes mage is available without verification.
backend: + @which mage > /dev/null || (echo "mage not found. Please install it first." && exit 1) mage -v build:linuxpkg/models/settings.go (1)
10-13: Consider field validation for Path.The Path field in PluginSettings has no validation or default values.
Add validation for the Path field in the LoadPluginSettings function:
func LoadPluginSettings(source backend.DataSourceInstanceSettings) (*PluginSettings, error) { settings := PluginSettings{} err := json.Unmarshal(source.JSONData, &settings) if err != nil { return nil, fmt.Errorf("could not unmarshal PluginSettings json: %w", err) } + // Validate Path + if settings.Path == "" { + backend.Logger.Info("Path not specified, using default") + settings.Path = "/api" + } backend.Logger.Debug("loading settings") settings.Secrets = loadSecretPluginSettings(source.DecryptedSecureJSONData) return &settings, nil }src/plugin.json (1)
40-46: Consider making the route URL configurable.
Hardcoding the route"https://api.example.com/api/v1/tables"might be limiting in real-world deployments. Storing the base URL in plugin settings or environment variables could improve maintainability.- "url": "https://api.example.com/api/v1/tables", + "url": "%CUSTOM_BASE_URL%/api/v1/tables",pkg/plugin/datasource.go (2)
29-55: Synchronize user-agent with plugin metadata.
Currently, the user-agent is set to"gtom/1.0". Consider making it consistent with the plugin name or version so that logs and analytics can clearly identify traffic sources.- opts.Header.Add("User-Agent", "gtom/1.0") + opts.Header.Add("User-Agent", "jsonds_backend/1.0")
108-114: Dispose of any allocated resources.
The method is empty; ensure that any persistent connections or other resources are cleaned up here to prevent resource leaks in long-running environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.gitignore(1 hunks)Magefile.go(1 hunks)Makefile(1 hunks)go.mod(1 hunks)pkg/main.go(1 hunks)pkg/models/settings.go(1 hunks)pkg/plugin/datasource.go(1 hunks)pkg/plugin/datasource_test.go(1 hunks)src/DataSource.ts(7 hunks)src/MetricFindQuery.ts(1 hunks)src/plugin.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/plugin/datasource_test.go (1)
pkg/plugin/datasource.go (1)
Datasource(59-62)
pkg/main.go (1)
pkg/plugin/datasource.go (1)
NewDatasource(30-55)
src/DataSource.ts (2)
src/types.d.ts (3)
GrafanaQuery(6-11)GenericOptions(13-15)MetricConfig(43-47)src/doFetch.ts (1)
doFetch(4-9)
🔇 Additional comments (14)
go.mod (1)
5-5: LGTM - Using appropriate Grafana Plugin SDK versionThe dependency on
github.com/grafana/grafana-plugin-sdk-gois correctly specified with a specific version number, which is good practice for ensuring reproducible builds.Magefile.go (2)
1-9: LGTM - Appropriate Mage configurationThe file correctly sets up the Mage build system with the proper build tags and imports the Grafana Plugin SDK build package, following standard patterns for Grafana plugin development.
11-12: LGTM - Default build target configured correctlySetting the default target to
build.BuildAllis appropriate as it provides a complete build process using the Grafana SDK's build utilities..gitignore (1)
15-16: LGTM - Appropriate gitignore rule for plugin packagesAdding
*.zipto the gitignore file is appropriate since Grafana plugins are typically packaged as zip files for distribution. The comment clearly explains the purpose of this rule.src/MetricFindQuery.ts (1)
26-27: The URL format has been updated to use a more standardized Grafana API endpoint structure.The change from a direct URL to using the datasource UID in the format
/api/datasources/uid/${this.datasource.uid}/resources/variablealigns with Grafana's convention for accessing datasource-specific resources via the backend.src/plugin.json (1)
7-9: Enabling alerting and backend looks good.
These flags allow the datasource to support both alerting and backend functionality, aligning with Grafana's guidelines.pkg/plugin/datasource.go (2)
115-133: QueryData method seems sufficiently robust.
Looping over queries and gathering results in a map is standard practice. No concerns here.
135-165: Example query logic is acceptable.
This method returns dummy data to illustrate the structure. Confirm that the final implementation will populate frames with real data. Otherwise, no issues noted.src/DataSource.ts (6)
10-10: Switch to DataSourceWithBackend is appropriate.
This import reflects the recommended modern approach for backend-enabled data sources.
32-32: Extending DataSourceWithBackend looks correct.
Using the built-in backend integration simplifies data query and health-check handling.
85-99: POST route for metric-payload-options appears correct.
Leverages the backend route for retrieving payload options. Good usage of the doFetch utility.
102-134: listMetrics method updated to async and using the new route.
Making it async is sensible, and the new/metricsroute aligns with your backend implementation.
139-144: Using the new route for tag-keys looks fine.
The updated endpoint is consistent with the backend resource calls.
149-153: Updated route for tag-values is concise.
Implementation seamlessly switches to the backend resource, matching the approach in tag-keys.
| @@ -0,0 +1,92 @@ | |||
| module github.com/simPod/GrafanaJsonDatasource | |||
|
|
|||
| go 1.23 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Go version compatibility
The module specifies Go 1.23, which appears to be a future version (as of April 2024, Go 1.22 is the latest stable release). This might cause compatibility issues when building or running the plugin in environments using currently available Go versions.
Consider using a released Go version (e.g., 1.22) to ensure wider compatibility. Please verify the required Go version for the Grafana Plugin SDK being used.
🌐 Web query:
What is the latest stable version of Go as of April 2024?
💡 Result:
The latest stable version of Go as of April 2024 was Go 1.22. The most recent minor release at that time was Go 1.22.2, issued on April 3, 2024[7].
Key details about Go 1.22:
- Introduced safer loop variable semantics to prevent accidental sharing bugs[10].
- Added support for ranging over integers and HTTP routing pattern enhancements[10].
- Included updates to the
math/rand/v2package and newgo/versionpackage[10].
This release adhered to Go’s six-month major release cycle, with Go 1.23 following in August 2024[1][7]. As per Go’s support policy, each major version is supported until two subsequent releases are available, ensuring critical fixes for approximately one year[1][7].
Citations:
- 1: https://endoflife.date/go
- 2: http://xahlee.info/golang/golang_version_dates.html
- 3: https://cloud.google.com/appengine/docs/standard/go/release-notes
- 4: https://tip.golang.org/doc/go1.24
- 5: https://en.wikipedia.org/wiki/Go_(programming_language)
- 6: https://gist.github.com/nikhita/432436d570b89cab172dcf2894465753
- 7: https://go.dev/doc/devel/release
- 8: https://www.bytesizego.com/blog/go-language-history
- 9: https://github.com/golang/tools/releases
- 10: https://antonz.org/which-go/
Action: Adjust Go version in go.mod for compatibility
- The
go.modfile currently specifies:go 1.23 - As verified, the latest stable version of Go as of April 2024 is 1.22 (with 1.22.2 as the current minor release). Using Go 1.23 may lead to compatibility issues in environments where only the stable version (1.22) is available.
- Please confirm that the Grafana Plugin SDK doesn’t require any features exclusive to a newer version and update the Go version in
go.modto a released version (e.g.,go 1.22) if no specific advantages are utilized.
| ) | ||
|
|
||
| func TestQueryData(t *testing.T) { | ||
| ds := Datasource{} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize Datasource with meaningful configuration
Creating a Datasource with empty values may not accurately reflect real usage patterns and could lead to misleading test results.
The Datasource appears to require a URL and an HTTP client to function correctly. Initialize these fields with appropriate values for testing:
-ds := Datasource{}
+ds := Datasource{
+ url: "http://example.com",
+ httpClient: &http.Client{},
+}Remember to add the corresponding imports:
import "net/http"| func TestQueryData(t *testing.T) { | ||
| ds := Datasource{} | ||
|
|
||
| resp, err := ds.QueryData( | ||
| context.Background(), | ||
| &backend.QueryDataRequest{ | ||
| Queries: []backend.DataQuery{ | ||
| {RefID: "A"}, | ||
| }, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| t.Error(err) | ||
| } | ||
|
|
||
| if len(resp.Responses) != 1 { | ||
| t.Fatal("QueryData must return a response") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with more thorough assertions
While the test successfully verifies that a response is returned, it doesn't validate the contents of the response or test different scenarios.
Consider enhancing the test with:
- Validation of response content (e.g., status, values)
- Testing with different query parameters
- Testing error scenarios
- Mocking HTTP responses to avoid external dependencies
Example improvement:
func TestQueryData(t *testing.T) {
- ds := Datasource{}
+ // Setup mock HTTP client to avoid real HTTP calls
+ mockClient := &http.Client{
+ Transport: &mockTransport{},
+ }
+ ds := Datasource{
+ url: "http://example.com",
+ httpClient: mockClient,
+ }
resp, err := ds.QueryData(
context.Background(),
&backend.QueryDataRequest{
Queries: []backend.DataQuery{
{RefID: "A"},
},
},
)
if err != nil {
t.Error(err)
}
if len(resp.Responses) != 1 {
t.Fatal("QueryData must return a response")
}
+ // Verify response for query "A"
+ queryResp, ok := resp.Responses["A"]
+ if !ok {
+ t.Fatal("Expected response for query with RefID 'A'")
+ }
+ // Verify status
+ if queryResp.Status != backend.StatusOK {
+ t.Errorf("Expected status OK, got %v", queryResp.Status)
+ }
+ // Additional assertions for response content
}
+// Mock HTTP transport for testing
+type mockTransport struct{}
+
+func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+ // Return a mock response based on the request
+ return &http.Response{
+ StatusCode: 200,
+ Body: io.NopCloser(bytes.NewBufferString(`{"some": "data"}`)),
+ Header: make(http.Header),
+ }, nil
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestQueryData(t *testing.T) { | |
| ds := Datasource{} | |
| resp, err := ds.QueryData( | |
| context.Background(), | |
| &backend.QueryDataRequest{ | |
| Queries: []backend.DataQuery{ | |
| {RefID: "A"}, | |
| }, | |
| }, | |
| ) | |
| if err != nil { | |
| t.Error(err) | |
| } | |
| if len(resp.Responses) != 1 { | |
| t.Fatal("QueryData must return a response") | |
| } | |
| } | |
| func TestQueryData(t *testing.T) { | |
| // Setup mock HTTP client to avoid real HTTP calls | |
| mockClient := &http.Client{ | |
| Transport: &mockTransport{}, | |
| } | |
| ds := Datasource{ | |
| url: "http://example.com", | |
| httpClient: mockClient, | |
| } | |
| resp, err := ds.QueryData( | |
| context.Background(), | |
| &backend.QueryDataRequest{ | |
| Queries: []backend.DataQuery{ | |
| {RefID: "A"}, | |
| }, | |
| }, | |
| ) | |
| if err != nil { | |
| t.Error(err) | |
| } | |
| if len(resp.Responses) != 1 { | |
| t.Fatal("QueryData must return a response") | |
| } | |
| // Verify response for query "A" | |
| queryResp, ok := resp.Responses["A"] | |
| if !ok { | |
| t.Fatal("Expected response for query with RefID 'A'") | |
| } | |
| // Verify status | |
| if queryResp.Status != backend.StatusOK { | |
| t.Errorf("Expected status OK, got %v", queryResp.Status) | |
| } | |
| // Additional assertions for response content | |
| } | |
| // Mock HTTP transport for testing | |
| type mockTransport struct{} | |
| func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |
| // Return a mock response based on the request | |
| return &http.Response{ | |
| StatusCode: 200, | |
| Body: io.NopCloser(bytes.NewBufferString(`{"some": "data"}`)), | |
| Header: make(http.Header), | |
| }, nil | |
| } |
| func main() { | ||
| // Start listening to requests sent from Grafana. This call is blocking so | ||
| // it won't finish until Grafana shuts down the process or the plugin choose | ||
| // to exit by itself using os.Exit. Manage automatically manages life cycle | ||
| // of datasource instances. It accepts datasource instance factory as first | ||
| // argument. This factory will be automatically called on incoming request | ||
| // from Grafana to create different instances of SampleDatasource (per datasource | ||
| // ID). When datasource configuration changed Dispose method will be called and | ||
| // new datasource instance created using NewSampleDatasource factory. | ||
| log.DefaultLogger.Error("loading jsonds-backend") | ||
| if err := datasource.Manage("jsonds-backend", plugin.NewDatasource, datasource.ManageOpts{}); err != nil { | ||
| log.DefaultLogger.Error(err.Error()) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The main function properly initializes the datasource plugin, but contains incorrect log level usage.
The implementation correctly sets up the plugin lifecycle using the Grafana SDK's datasource management. However, there's an issue with the logging.
- log.DefaultLogger.Error("loading jsonds-backend")
+ log.DefaultLogger.Info("loading jsonds-backend")Using Error level for informational messages can cause confusion when monitoring logs, as it suggests there's a problem when there isn't one.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func main() { | |
| // Start listening to requests sent from Grafana. This call is blocking so | |
| // it won't finish until Grafana shuts down the process or the plugin choose | |
| // to exit by itself using os.Exit. Manage automatically manages life cycle | |
| // of datasource instances. It accepts datasource instance factory as first | |
| // argument. This factory will be automatically called on incoming request | |
| // from Grafana to create different instances of SampleDatasource (per datasource | |
| // ID). When datasource configuration changed Dispose method will be called and | |
| // new datasource instance created using NewSampleDatasource factory. | |
| log.DefaultLogger.Error("loading jsonds-backend") | |
| if err := datasource.Manage("jsonds-backend", plugin.NewDatasource, datasource.ManageOpts{}); err != nil { | |
| log.DefaultLogger.Error(err.Error()) | |
| os.Exit(1) | |
| } | |
| func main() { | |
| // Start listening to requests sent from Grafana. This call is blocking so | |
| // it won't finish until Grafana shuts down the process or the plugin choose | |
| // to exit by itself using os.Exit. Manage automatically manages life cycle | |
| // of datasource instances. It accepts datasource instance factory as first | |
| // argument. This factory will be automatically called on incoming request | |
| // from Grafana to create different instances of SampleDatasource (per datasource | |
| // ID). When datasource configuration changed Dispose method will be called and | |
| // new datasource instance created using NewSampleDatasource factory. | |
| log.DefaultLogger.Info("loading jsonds-backend") | |
| if err := datasource.Manage("jsonds-backend", plugin.NewDatasource, datasource.ManageOpts{}); err != nil { | |
| log.DefaultLogger.Error(err.Error()) | |
| os.Exit(1) | |
| } | |
| } |
| CC := go | ||
| PKG_NAME := "uam_gtom" | ||
| VERSION := "1.0.0" | ||
| TRASH := dist $(PKG_NAME) *.zip |
There was a problem hiding this comment.
Package name inconsistency and versioning concerns.
The package name "uam_gtom" doesn't match the repository name "GrafanaJsonDatasource" or the backend plugin ID "jsonds-backend" used in pkg/main.go.
CC := go
- PKG_NAME := "uam_gtom"
+ PKG_NAME := "jsonds"
VERSION := "1.0.0"
TRASH := dist $(PKG_NAME) *.zipAlso consider implementing dynamic versioning rather than hardcoding the version.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CC := go | |
| PKG_NAME := "uam_gtom" | |
| VERSION := "1.0.0" | |
| TRASH := dist $(PKG_NAME) *.zip | |
| CC := go | |
| PKG_NAME := "jsonds" | |
| VERSION := "1.0.0" | |
| TRASH := dist $(PKG_NAME) *.zip |
| func loadSecretPluginSettings(source map[string]string) *SecretPluginSettings { | ||
| return &SecretPluginSettings{ | ||
| ApiKey: source["apiKey"], | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing validation for API key.
The function loads the API key without validating that it exists or isn't empty.
func loadSecretPluginSettings(source map[string]string) *SecretPluginSettings {
+ apiKey := source["apiKey"]
+ if apiKey == "" {
+ backend.Logger.Warn("API key is empty")
+ }
return &SecretPluginSettings{
- ApiKey: source["apiKey"],
+ ApiKey: apiKey,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func loadSecretPluginSettings(source map[string]string) *SecretPluginSettings { | |
| return &SecretPluginSettings{ | |
| ApiKey: source["apiKey"], | |
| } | |
| } | |
| func loadSecretPluginSettings(source map[string]string) *SecretPluginSettings { | |
| apiKey := source["apiKey"] | |
| if apiKey == "" { | |
| backend.Logger.Warn("API key is empty") | |
| } | |
| return &SecretPluginSettings{ | |
| ApiKey: apiKey, | |
| } | |
| } |
| return nil, fmt.Errorf("could not unmarshal PluginSettings json: %w", err) | ||
| } | ||
|
|
||
| backend.Logger.Error("loading settings", "source", source) |
There was a problem hiding this comment.
Incorrect log level usage and potential security risk.
Using error level for an informational message is inappropriate. Additionally, logging the entire source object might expose sensitive information.
- backend.Logger.Error("loading settings", "source", source)
+ backend.Logger.Debug("loading settings")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backend.Logger.Error("loading settings", "source", source) | |
| backend.Logger.Debug("loading settings") |
| // Check https://grafana.com/developers/plugin-tools/create-a-plugin/extend-a-plugin/add-resource-handler | ||
| func (d *Datasource) CallResource(ctx context.Context, req *backend.CallResourceRequest, sender backend.CallResourceResponseSender) error { | ||
| backend.Logger.Debug("handling a resource query", "path", req.Path) | ||
| switch req.Path { | ||
| case "metrics", "metric-payload-options", "variable", "tag-keys", "tag-values": | ||
| backend.Logger.Debug("handling metrics query", "body", req.Body) | ||
|
|
||
| resp, err := d.httpClient.Post(d.url+"/"+req.Path, "application/json", bytes.NewReader(req.Body)) | ||
| if err != nil { | ||
| backend.Logger.Error("couldn't get metrics", "err", err) | ||
| return sender.Send(&backend.CallResourceResponse{ | ||
| Status: http.StatusNotFound, | ||
| }) | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| backend.Logger.Error("couldn't get metrics", "rc", resp.StatusCode) | ||
| return sender.Send(&backend.CallResourceResponse{ | ||
| Status: http.StatusNotFound, | ||
| Body: []byte(`{"err": "couldn't get the metrics"}`), | ||
| }) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| backend.Logger.Error("couldn't read the returned metrics", "err", err) | ||
| return sender.Send(&backend.CallResourceResponse{ | ||
| Status: http.StatusNotFound, | ||
| Body: []byte(`{"err": "couldn't read the returned metrics"}`), | ||
| }) | ||
| } | ||
|
|
||
| return sender.Send(&backend.CallResourceResponse{ | ||
| Status: http.StatusOK, | ||
| Body: body, | ||
| }) | ||
| default: | ||
| return sender.Send(&backend.CallResourceResponse{ | ||
| Status: http.StatusNotFound, | ||
| Body: []byte(`{"err": "requested non-existent path"}`), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve remote HTTP status codes for better error handling.
Currently, if the external service call fails or returns a non-200 status, a 404 is sent back, which may be misleading. Forwarding or mapping the status code to an appropriate HTTP response would improve troubleshooting.
- return sender.Send(&backend.CallResourceResponse{
- Status: http.StatusNotFound,
- })
...
- return sender.Send(&backend.CallResourceResponse{
- Status: http.StatusNotFound,
- Body: []byte(`{"err": "couldn't get the metrics"}`),
- })
+ return sender.Send(&backend.CallResourceResponse{
+ Status: resp.StatusCode,
+ Body: []byte(fmt.Sprintf(`{"err": "received status code %d"}`, resp.StatusCode)),
+ })Committable suggestion skipped: line range outside the PR's diff.
|
This pull request has been automatically marked as stale because it |
Hi @simPod and @KrisztianKaszas! I'm opening up this PR to continue working on what was set forth in #630.
Sorry for the delay... things have a tendency to be pushed down in my inbox!
Let's see if we can push this through...
Summary by CodeRabbit
New Features
Refactor