feat: Adds time_format param for httpd#26596
Conversation
* This PR will add a time_format parameter which takes in the value "epoch" or "rfc3339". The default will be "epoch" depending on the value output timestamps will be formatted in epoch or rfc3339. Closes FR#615
3d14493 to
7eb766f
Compare
| // timeFormat should default to "epoch" | ||
| timeFormat := strings.TrimSpace(r.FormValue("time_format")) | ||
| if timeFormat == "" { | ||
| timeFormat = "epoch" | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it should return an error if an invalid value is given for time_format. This will save a lot of support time.
services/httpd/handler.go
Outdated
| if timeFormat == "rfc3339" { | ||
| convertToRfc3339(r) | ||
| } |
There was a problem hiding this comment.
It looks like we are pretty consistent in calling ns-precision RFC3339 simply RFC3339 in our docs and in the API. I wonder if we should allow rfc3339nano as a synonym?
| if epoch != "" { | ||
| if epoch != "" && timeFormat == "epoch" { | ||
| convertToEpoch(r, epoch) | ||
| } | ||
|
|
||
| if timeFormat == "rfc3339" { | ||
| convertToRfc3339(r) | ||
| } |
There was a problem hiding this comment.
Are these two mutually exclusive? If so, it should probably be an if-then-else.
It seems like setting time_format=rfc3339 and epoch=foo should give the user an error to avoid support calls of people trying to combine them to get RFC3339 with second precision or such.
services/httpd/handler_test.go
Outdated
| } | ||
|
|
||
| // Ensure the handler returns results with RFC3339 timestamp format for multiple series. | ||
| func TestHandler_Query_RFC3339_MultipleSeries(t *testing.T) { |
There was a problem hiding this comment.
If it was me, I would combine the two test functions to avoid copying and pasting the boilerplate.
* error if incorrect param * update naming for converter function * combine tests
services/httpd/handler.go
Outdated
| if timeFormat == "" { | ||
| timeFormat = "epoch" | ||
| } else if !slices.Contains(timeFormats, timeFormat) { | ||
| h.httpError(rw, `time format must be one of the following: "epoch", "rfc3339"`, http.StatusBadRequest) |
There was a problem hiding this comment.
Take advantage of the timeFormats that's already defined (and fix the parameter name). Example:
h.httpError(rw, fmt.Sprintf("time_format must be one of the following: %s", strings.Join(timeFormats, ",")), http.StatusBadRequest)
services/httpd/handler.go
Outdated
| func convertToRfc3339Nano(r *query.Result) { | ||
| for _, s := range r.Series { | ||
| for _, v := range s.Values { | ||
| if ts, ok := v[0].(time.Time); ok { | ||
| v[0] = ts.Format(time.RFC3339Nano) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Not required, but the time format (time.RFC3339Nano) could be made a parameter instead of hard-coded. The call would then be something like convertToTimeFormat(r, time.RFC3339Nano). The code would be ready to add different time formats later if requested.
convertToEpoch is sufficiently different that trying to combine a convertToTimeFormat with it would probably make the code harder to read. It's possible by passing the conversion function as a function parameter, but I don't think that would really improve the code.
services/httpd/handler_test.go
Outdated
| name: "single series", | ||
| testFunc: func(t *testing.T) { | ||
| h := NewHandler(false) | ||
| testTime := time.Date(2021, 1, 1, 12, 0, 0, 0, time.UTC) | ||
|
|
||
| h.StatementExecutor.ExecuteStatementFn = func(stmt influxql.Statement, ctx *query.ExecutionContext) error { | ||
| ctx.Results <- &query.Result{ | ||
| StatementID: 1, | ||
| Series: []*models.Row{{ | ||
| Name: "series0", | ||
| Columns: []string{"time", "value"}, | ||
| Values: [][]interface{}{ | ||
| {testTime, 42}, | ||
| }, | ||
| }}, | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
What I was thinking was that all the tests could use the same data set. They're almost doing that already. There's also still a lot of the same boilerplate code around the test bodies.
gwossum
left a comment
There was a problem hiding this comment.
Added some comments on the latest commits.
* Rename convertToRfc3339Nano to convertToTimeFormat * allow time formatting to be passed as parameter * adjust error handling to use already defined timeFormats * merge test data for test cases to reduce boilerplate
* feat: Adds time_format param for httpd * This PR will add a time_format parameter which takes in the value "epoch" or "rfc3339". The default will be "epoch" depending on the value output timestamps will be formatted in epoch or rfc3339. Closes FR#615 * feat: Adding some changes * error if incorrect param * update naming for converter function * combine tests * chore: fmt'ing * feat: A few modifications * Rename convertToRfc3339Nano to convertToTimeFormat * allow time formatting to be passed as parameter * adjust error handling to use already defined timeFormats * merge test data for test cases to reduce boilerplate (cherry picked from commit 57da7aa)
* feat: Adds time_format param for httpd * This PR will add a time_format parameter which takes in the value "epoch" or "rfc3339". The default will be "epoch" depending on the value output timestamps will be formatted in epoch or rfc3339. Closes FR#615 * feat: Adding some changes * error if incorrect param * update naming for converter function * combine tests * chore: fmt'ing * feat: A few modifications * Rename convertToRfc3339Nano to convertToTimeFormat * allow time formatting to be passed as parameter * adjust error handling to use already defined timeFormats * merge test data for test cases to reduce boilerplate (cherry picked from commit 57da7aa)
time_formatparameter which takes in the value "epoch" or "rfc3339". The default will be "epoch" depending on the value output timestamps will be formatted in epoch or rfc3339.Closes FR#615