-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Use storage package for Prometheus remote read #9967
Conversation
* Update Prometheus remote write to use metric name as measurement name and value as the field name. * Update Prometheus remote read to use the storage.Read method to bypass the InfluxQL query engine.
bccec99
to
1eca009
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Edd 🍻 Nice work making it more testable too!
Only one change required to fix a cursor leak and a couple of blank space nits.
services/httpd/handler.go
Outdated
} | ||
cur.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cur.Close()
should be moved outside the switch
to ensure all cursor types are closed or it will result in TSM file handle leaks.
services/httpd/handler_test.go
Outdated
"github.com/google/go-cmp/cmp" | ||
|
||
"github.com/influxdata/influxdb/logger" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank spaces
tests/server_test.go
Outdated
"strconv" | ||
"strings" | ||
"sync" | ||
"testing" | ||
"time" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank space
services/httpd/handler.go
Outdated
@@ -108,6 +110,11 @@ type Handler struct { | |||
WritePoints(database, retentionPolicy string, consistencyLevel models.ConsistencyLevel, user meta.User, points []models.Point) error | |||
} | |||
|
|||
Store interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a strong preference for inline interface definitions vs promoting it to a package-level type? i.e.
type Store interface {
Read(ctx context.Context, req *storage.ReadRequest) (storage.Results, error)
WithLogger(log *zap.Logger)
}
My case is that some tools appear to have a harder time identifying inlined interfaces and subsequently other types which implement this interface.
8b93cfe
to
3cb9e13
Compare
@stuartcarnie feedback addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mate 🎉
Use storage package for Prometheus remote read
Use storage package for Prometheus remote read
Use storage package for Prometheus remote read
Use storage package for Prometheus remote read
This PR converts the Prometheus remote read endpoint to use the
storage
package, rather than the InfluxQL query engine.This results in a number of improvements in performance and efficiency.
The PR also fixes a bug in the
storage
package, where regex expressions were not being properly handled for storage requests.Finally it adds some higher-level end-to-end tests in the
tests
package.