Karios db executor #12
Conversation
… Kairosdb query build in progress.
…fana model and convert it to kairos api. Also added api's to get the response from kairos metric query rest api and transform it to grafana model.
|
Please write unit tests for these. |
|
@ddhirajkumar : There is a sperate story to write unit tests for all the go files added as part of kairosdb executor. This will be taken up in next sprint. |
pkg/tsdb/kairosdb/kairosdb.go
Outdated
| } | ||
|
|
||
| func NewKairosdbExecutor(datasource *models.DataSource) (tsdb.Executor, error) { | ||
| plog.Info("Inside NewKairosdbExecutor - 0") |
There was a problem hiding this comment.
@vkhatale please remove these if no longer needed. Or convert them to debug logs. Whatever seems appropriate.
There was a problem hiding this comment.
@ddhirajkumar : Will change to debug
| result := &tsdb.BatchResult{} | ||
|
|
||
| kairosdbQuery := NewQueryBuilder() | ||
| kairosdbQuery.SetAbsoluteStart(queryContext.TimeRange.MustGetFrom().UTC()) |
There was a problem hiding this comment.
The time handling seems to be correct code-wise. But would advise to test with different formats from the UI in Grafana's time range field.
There was a problem hiding this comment.
Will test this when writing unit tests and also do manual testing to ensure this.
| queryRes.Series = append(queryRes.Series, &series) | ||
| } | ||
| } | ||
| queryResults["A"] = queryRes |
There was a problem hiding this comment.
What's the reason for this hardcoding to "A"?
There was a problem hiding this comment.
@ddhirajkumar : That is how it is done in all other executors. AFAIK it is because there is only one query allowed for alert execution. You can opentsdb.go, influxdb.go, prometheus.go....they all do the same way.
There was a problem hiding this comment.
I see... I digged up a further and found that when Grafana code makes a request to the executor, it refers to the query by the string "A". If interested, you can take a look at pkg/services/alerting/conditions/alerting.go#getRequestForAlertRule() function.
|
|
||
| func (e *KairosdbExecutor) createRequest(kairosdbQuery QueryBuilder) (*http.Request, error) { | ||
| u, _ := url.Parse(e.Url) | ||
| u.Path = path.Join(u.Path, "api/v1/datapoints/query/") |
There was a problem hiding this comment.
Does it make sense to move the url path to config.ini?
There was a problem hiding this comment.
In opentsdb the url path is defined here. I did same thing for Kairosdb. Moreover I don't see seperate sections in ini file to configure datasource specific configurations.
There was a problem hiding this comment.
That doesn't appear to be a reasonable justification. You can add a new section for karios datasource configuration (if it's not already there). Since this is code that we would be releasing to Opensource, would be good to have this as config instead of being hardcoded.
There was a problem hiding this comment.
In that case I will create seperate story to put this in config.ini file. Not sure how to add it in ini file. Please merge this for now
| return nil, err | ||
| } | ||
|
|
||
| if res.StatusCode/100 != 2 { |
There was a problem hiding this comment.
I like this check. Instead of checking for equality with 200.
pkg/tsdb/kairosdb/kairosdb.go
Outdated
| err = json.Unmarshal(body, queryResponse) | ||
|
|
||
| if err != nil { | ||
| plog.Info("Failed to unmarshal opentsdb response", "error", err, "status", res.Status, "body", string(body)) |
There was a problem hiding this comment.
typo in the log: please search for 'opentsb' across all files and replace them with kairosdb
There was a problem hiding this comment.
Also the log level should be 'error' or 'warning' here
There was a problem hiding this comment.
Verified that there is no other place where 'opentsdb' string is used. Changed log level from info to error.
Added kairos db executor to query kairos db api's