Allow specifying multiple filtersets. #113

knyar · 2019-04-04T18:29:08Z

This allows specifying multiple sets of filters that time series will be
checked against. It also switches to using promql native parser for
filters.

Existing --filter flag will keep working, but is now marked as
deprecated.

This makes #106 obsolete.

StevenYCChou · 2019-04-04T18:58:35Z

Thanks for contributing, @knyar! As this PR is large, we will take some time to review it. I will try to make this PR get reviewed within 1 day.

README.md

cmd/stackdriver-prometheus-sidecar/main.go

StevenYCChou · 2019-04-05T18:44:08Z

@fabxc I saw you implemented filter. I am interested what the motivation you only choose validating against regexp and labels.Matcher?

cmd/stackdriver-prometheus-sidecar/main.go

cmd/stackdriver-prometheus-sidecar/main_test.go

cmd/stackdriver-prometheus-sidecar/main.go

cmd/stackdriver-prometheus-sidecar/main_test.go

retrieval/series_cache.go

retrieval/series_cache_test.go

StevenYCChou · 2019-04-09T19:21:47Z

README.md

@@ -48,14 +48,18 @@ stackdriver-prometheus-sidecar --help

 #### Filters

-The `--filter` flag allows to provide filters which all series have to pass before being sent to Stackdriver. The flag may be repeated to provide several filters. Filters use the same syntax as the well-known PromQL label matchers, e.g.:
+The `--filterset` flag allows to provide filters which all series have to pass before being sent to Stackdriver. Filters use the same syntax as [Prometheus time series selectors](https://prometheus.io/docs/prometheus/latest/querying/basics/#time-series-selectors), e.g.:


I recheck Prometheus document again - we should be really specific on the selector we use.

I think from both design and implementation POV, it's reasonable that we only support "instant vector selectors", not "range vector selectors". "time series selectors" include both of them. The rationale for design is that we should start from minimum set of selectors useful for users - and from what we see, most of the users are using "instant vector selectors" only.

Action item here: change from "time series selectors" to "instant vector selectors", and link to https://prometheus.io/docs/prometheus/latest/querying/basics/#instant-vector-selectors .

Note that your implementation is using ParseMetricSelector, which only works with instant vector selector.

Changed, though I think 'time series selectors' would be more clear to someone who is not intimately familiar with Prometheus data model.

StevenYCChou · 2019-04-09T19:40:19Z

cmd/stackdriver-prometheus-sidecar/main.go

@@ -231,7 +232,10 @@ func main() {
 	a.Flag("web.listen-address", "Address to listen on for UI, API, and telemetry.").
 		Default("0.0.0.0:9091").StringVar(&cfg.listenAddress)

-	a.Flag("filter", "PromQL-style label matcher which must pass for a series to be forwarded to Stackdriver. May be repeated.").
+	a.Flag("filterset", "PromQL metric and label matcher which must pass for a series to be forwarded to Stackdriver. If repeated, the series must pass any filter set to be forwarded.").


I felt that filterset does not serve well for the value we accept - as the value of filterset is not just comma split string (which fits the name filterset more), but instant vector selector from PromQL.

Also filterset and filter are really similar, which may confuse our users. Can you find an alternative name here?

I agree that filterset is not a great name, though being related to filter makes it clear that both flags are controlling the same thing.

Do you like --whitelist or --metric better?

Thanks for coming up with the name. Whitelist sounds good for me.

knyar · 2019-04-10T10:59:42Z

Added another commit addressing comments; will squash commits later when you're happy with the changes.

StevenYCChou

Thanks for accomodating the feedback and provide your thoughts! The PR looks good so far, and I think the only thing left is the parameter naming for metric whitelist/filtering.

knyar · 2019-04-12T16:37:54Z

Renamed --filterset to --whitelist, PTAL.

StevenYCChou · 2019-04-15T03:38:56Z

Javier and I have chatted over the naming. He mentioned that whitelist may not be a good term. I looked up whitelist in English dictionaries:

"a list of people or things that are considered by a particular authority or group to be acceptable and that should be trusted" from Cambridge Dictionary
"A list of people or things considered to be acceptable or trustworthy" from Oxford Dictionary

As in both dictionaries, they are all defined related to authority/trustworthy, I agreed that whitelist is not a good term here, as we are considering "what user want to send" to Stackdriver API, not considering authority/trustworthy.

What do you think about using --include as user-facing parameter, and still use filterset as the variable inside the code, as include may not be a good name for variable inside the code?

knyar · 2019-04-15T10:36:22Z

What do you think about using --include as user-facing parameter, and still use filterset as the variable inside the code, as include may not be a good name for variable inside the code?

Sounds good, I've renamed the variables back to filterset, and the flag is now called --include.

StevenYCChou

LGTM, only one minor change for logging message before merging back.

Note: can you create new commit on top of existing commits instead of force pushing? In this way, code reviewer has option to do incremental code review by reviewing commit one by one.

cmd/stackdriver-prometheus-sidecar/main.go

This allows specifying multiple sets of filters that time series will be checked against. It also switches to using promql native parser for filters. Existing `--filter` flag will keep working, but is now marked as deprecated.

StevenYCChou · 2019-04-15T13:53:57Z

Thanks for contributing, @knyar ! I just merged this PR after reviewing your latest change on log message.

StevenYCChou self-requested a review April 5, 2019 15:10

StevenYCChou reviewed Apr 5, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

cmd/stackdriver-prometheus-sidecar/main.go Show resolved Hide resolved

StevenYCChou suggested changes Apr 9, 2019

View reviewed changes

cmd/stackdriver-prometheus-sidecar/main.go Outdated Show resolved Hide resolved

StevenYCChou reviewed Apr 9, 2019

View reviewed changes

StevenYCChou reviewed Apr 12, 2019

View reviewed changes

StevenYCChou approved these changes Apr 15, 2019

View reviewed changes

cmd/stackdriver-prometheus-sidecar/main.go Outdated Show resolved Hide resolved

Allow specifying multiple sets of filters.

a91924d

This allows specifying multiple sets of filters that time series will be checked against. It also switches to using promql native parser for filters. Existing `--filter` flag will keep working, but is now marked as deprecated.

StevenYCChou merged commit 6de434d into Stackdriver:master Apr 15, 2019

googlebot added the cla: yes label Apr 18, 2019

knyar mentioned this pull request Apr 26, 2019

Apply OR logic when multiple --filter parameters are specified. #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow specifying multiple filtersets. #113

Allow specifying multiple filtersets. #113

knyar commented Apr 4, 2019

StevenYCChou commented Apr 4, 2019

StevenYCChou commented Apr 5, 2019

StevenYCChou Apr 9, 2019

knyar Apr 10, 2019

StevenYCChou Apr 9, 2019

knyar Apr 10, 2019

StevenYCChou Apr 12, 2019

knyar commented Apr 10, 2019

StevenYCChou left a comment

knyar commented Apr 12, 2019

StevenYCChou commented Apr 15, 2019

knyar commented Apr 15, 2019

StevenYCChou left a comment

StevenYCChou commented Apr 15, 2019

Allow specifying multiple filtersets. #113

Allow specifying multiple filtersets. #113

Conversation

knyar commented Apr 4, 2019

StevenYCChou commented Apr 4, 2019

StevenYCChou commented Apr 5, 2019

StevenYCChou Apr 9, 2019

Choose a reason for hiding this comment

knyar Apr 10, 2019

Choose a reason for hiding this comment

StevenYCChou Apr 9, 2019

Choose a reason for hiding this comment

knyar Apr 10, 2019

Choose a reason for hiding this comment

StevenYCChou Apr 12, 2019

Choose a reason for hiding this comment

knyar commented Apr 10, 2019

StevenYCChou left a comment

Choose a reason for hiding this comment

knyar commented Apr 12, 2019

StevenYCChou commented Apr 15, 2019

knyar commented Apr 15, 2019

StevenYCChou left a comment

Choose a reason for hiding this comment

StevenYCChou commented Apr 15, 2019