-
Notifications
You must be signed in to change notification settings - Fork 455
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
[query] Custom handler support #2073
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2073 +/- ##
========================================
- Coverage 72.5% 72.4% -0.2%
========================================
Files 1005 1007 +2
Lines 86287 86395 +108
========================================
- Hits 62584 62552 -32
- Misses 19530 19669 +139
- Partials 4173 4174 +1
Continue to review full report at Codecov.
|
@@ -42,6 +42,11 @@ const ( | |||
// of propagating aggregator placement to clients, usually needed when there is | |||
// a large amount of clients sending traffic to m3aggregator. | |||
defaultM3AggWarmupDuration = 0 | |||
|
|||
// DefaultServiceEnvironment is the default service ID environment. | |||
DefaultServiceEnvironment = "default_env" |
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.
Are these already declared somewhere else or were they moved?
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.
Yeah, it was moved from headers.go
; moved it back 👍
} | ||
|
||
handler := httpd.NewHandler(handlerOptions, runOpts.CustomHandlers...) |
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.
Nice 👍
src/query/executor/options.go
Outdated
"github.com/m3db/m3/src/query/storage" | ||
|
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.
nit: Unnecessary end line between imports?
// MetricsTypeHeader sets the write or read metrics type to restrict | ||
// metrics to. | ||
// Valid values are "unaggregated" or "aggregated". | ||
MetricsTypeHeader = "M3-Metrics-Type" |
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 these need to be moved back to headers.go
or staying here?
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 other than final minor comments
0310f08
to
842a9e1
Compare
Name: nameBytes, | ||
Value: matchValues, | ||
Type: models.MatchField, | ||
Name: nameBytes, |
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.
Perhaps remove var matchValues = []byte(".*")
? Looks like it's not used anymore.
src/query/server/query.go
Outdated
CustomHandlers []options.CustomHandler | ||
|
||
// CustomParseFunction is a custom parsing function. | ||
CustomParseFunction promql.ParseFn |
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.
nit: Should we call this CustomPromQLParseFunction
? In case we need to add another language, etc which may cause a backwards incompat change for those using this code?
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 with minor comments
What this PR does / why we need it:
Allows custom 3rd party handlers.
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: