SEARCH-6490 Migrate to backend, add support for alerting #20
Conversation
dcheckoway
force-pushed
the
dc/SEARCH-6490-migrate-to-backend-and-support-alerting
branch
4 times, most recently
from
5c0e6e6 to
7063250
Compare
dcheckoway
force-pushed
the
dc/SEARCH-6490-migrate-to-backend-and-support-alerting
branch
from
7063250 to
3e6381b
Compare
| } | ||
|
|
||
| type SecretPluginSettings struct { | ||
| ClientSecret string `json:"clientSecret"` |
There was a problem hiding this comment.
Probably a Go noob question, why ignore the field with json:"-" for PluginSettings.Secrets, but ClientSecret isn't also json:"-"?
There was a problem hiding this comment.
Great question...this came from Grafana's codegen. Lemme look into this...
There was a problem hiding this comment.
@itsalvinho the json:"-" isn't ignored. That's a catch-all. And there isn't a catch-all in SecretPluginSettings (just the one explicitly named clientSecret field) since we don't expect any other fields.
pkg/plugin/search_api.go
Outdated
| Items []map[string]any `json:"items"` | ||
| } | ||
| err = json.Unmarshal(responseBytes, &data) | ||
| if err != nil { |
There was a problem hiding this comment.
nit: I guess you could collapse this into if err := json.Unmarshal(responseBytes, &data); err != nil
There was a problem hiding this comment.
Definitely, will do!
There was a problem hiding this comment.
@itsalvinho in this case you can't "initialize" err directly with := since has already been defined. But you CAN collapse into one line (which I'll do).
| publicPath: `public/plugins/${pluginJson.id}/fonts/`, | ||
| outputPath: 'fonts/', | ||
| filename: Boolean(env.production) ? '[hash][ext]' : '[name][ext]', | ||
| filename: Boolean(env.production) ? '[hash][ext]' : '[file]', |
There was a problem hiding this comment.
Wouldn't this just be the same as env.production without the Boolean around it? What types of values do we expect for env.production?
There was a problem hiding this comment.
This is another boilerplate change straight from Grafana
package.json
Outdated
| "sign": "npx --yes @grafana/sign-plugin@latest" | ||
| }, | ||
| "author": "Cribl, Inc.", | ||
| "author": "Criblcloud", |
There was a problem hiding this comment.
Wondering about the author change here?
There was a problem hiding this comment.
Good catch! The way I did this was to create a brand new plugin from scratch using Grafana's NPX toolkit. So a lot of this was just "copied over" the old one. This is one of those cases.
Thanks for scrutinizing all of it. I'll fix this.
| const MAX_RESULTS = 10000 // same as what the actual Cribl UI imposes | ||
| const QUERY_PAGE_SIZE = 1000 | ||
| const CRIBL_TIME_FIELD = "_time" | ||
| const MAX_DELAY_WAITING_FOR_FINISHED = 60 * time.Second // could make this configurable in a future release |
There was a problem hiding this comment.
Is this for all queries, including those driving alerts? If so, wondering if eventually we may want two max delays - a second one for the background queries driving alerts, etc.
There was a problem hiding this comment.
All queries. AFAIK there's no way to know whether the query is interactive or not. I'll look into it to confirm.
|
|
||
| return response, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
NIT: Noticed other function names are CamelCase but this one is lower.
There was a problem hiding this comment.
It's a Go convention. PublicMethod vs. privateMethod.
| }, | ||
| []string{"query_type"}, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Which others? This is straight out of Grafana codegen.
| // an attempt to provide the most user-friendly representation of the error. | ||
| func parseErrorFromResponse(body []byte) error { | ||
| var jsonBody map[string]interface{} | ||
| if err := json.Unmarshal(body, &jsonBody); err != nil { |
There was a problem hiding this comment.
Is this expected, or should this return an error? Or would that be compensated for by the caller?
There was a problem hiding this comment.
I specifically decided not to return (error, error) here. I felt that would be royally confusing. This is a function that tries to parse an error, and "failure" here is deliberately indicated by nil.
The equivalent of return undefined in TS. This is one of those times when Go conventions can get in the way of clarity.
There was a problem hiding this comment.
BTW, in case it wasn't clear, if there's an error trying to unmarshal the JSON, it's not well-formed JSON, so we're returning nil to indicate that we couldn't parse an error from the JSON (that's not JSON). :-)
dcheckoway
deleted the
dc/SEARCH-6490-migrate-to-backend-and-support-alerting
branch
Followed this guide: https://grafana.com/developers/plugin-tools/how-to-guides/data-source-plugins/convert-a-frontend-datasource-to-backend
Effectively there's no change in functionality, just the migration to the backend, and all the core datasource functionality living in Go now under
pkg.I've wrung it out but let's test the heck out of it before we merge it.