[query] Remote storages can warn rather than error on failure #1938

arnikola · 2019-09-12T18:51:30Z

What this PR does / why we need it:
Previously any failing remote storage calls would cause the entire fanout query to fail. This PR sets default remote storage options to warn users but keep going, with an option to override it and set remotes as necessary.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

src/cmd/services/m3query/config/config.go

robskillington · 2019-09-12T19:42:56Z

src/cmd/services/m3query/config/remote_options.go

+) Remote {
+	if override != nil {
+		global = *override
+	}


nit(ignore if you'd like): Any reason to do this in the constructor, than perhaps just in the for loop creating the remotes?

Had to follow to next method call to work out what the ... defaultBehavior, nil) meant (i.e. what is nil param used for).

Ah yeah missed responding to this; tried it both ways and this looked a little cleaner

robskillington · 2019-09-12T19:46:51Z

src/query/server/server.go

 			fmt.Fprintf(os.Stderr, "unable to load %s: %v", runOpts.ConfigFiles, err)
 			os.Exit(1)
 		}
-		fmt.Fprintf(os.Stdout, "using %s config files: %v", serviceName, runOpts.ConfigFiles)
+
+		fmt.Fprintf(os.Stdout, "using %s config files: %v",


Hm, I haven't seen this fmt.Printf before - should we perhaps not be printing out this out? Seems strange and don't think any of our other servers do it.

Looks like it was added to help debug an issue where the wrong config was getting picked up #1803 (fixing #1801)

If we don't need it, happy to get rid of it though

src/query/server/server.go

robskillington · 2019-09-12T19:48:55Z

src/query/server/server.go

-		remotes, enabled, err := remoteClient(cfg, tagOptions, poolWrapper,
-			readWorkerPool, instrumentOptions)
+		opts := config.RemoteOptionsFromConfig(cfg.RPC)
+		lookback := *cfg.LookbackDuration


Hm, isn't it safer to use cfg.LookbackDurationOrDefault() here?

Yeah I was pretty bleh about the way this whole thing is setup, but its guaranteed to be safe here since this happens just above: https://sourcegraph.com/github.com/m3db/m3@arnikola/remote-error-behavior/-/blob/src/query/server/server.go#L218:29

I didn't want to just pull it out since some stuff depends on that lookback duration being set; wouldn't mind refactoring it a bit but wanted to stay more focused here

Although looks like I can use lookbackDuration here at least

robskillington · 2019-09-12T19:52:07Z

src/query/storage/error_behavior.go

+import "fmt"
+
+var (
+	// NB: Container behavior must not be defined in configs, so it does not count


Interesting. Is it worth adding what a container behavior is? Haven't managed to grok what that is, might be worth mentioning here too.

Sure, sounds good. Might change name to composite or something, basically it's only for fanout.Storage

robskillington · 2019-09-12T19:56:56Z

src/query/storage/fanout/storage.go

+						zap.Error(err),
+						zap.String("store", store.Name()),
+						zap.String("function", "FetchBlocks"))
+				}


We could make this slightly more go like:

if warning, err := storage.IsWarning(store, err); warning { numWarning++ s.instrumentOpts.Logger().Warn( "partial results: fanout to store returned warning", zap.Error(err), zap.String("store", store.Name()), zap.String("function", "FetchBlocks")) return } multiErr = multiErr.Add(err) s.instrumentOpts.Logger().Warn( "fanout to store returned error", zap.Error(err), zap.String("store", store.Name()), zap.String("function", "FetchBlocks")) return

Sure, looks a little weird to me but if that's the convention easy times

robskillington · 2019-09-12T19:59:22Z

src/query/storage/fanout/storage.go

+					zap.String("function", "SearchSeries"))
+
+				return nil, err
+			}


Hm thinking you're missing a "continue" for the warning case (since otherwise metrics = append(metrics, result.Metrics...) will likely be a nil pointer error?).

Also to make it more go like, maybe this:

if warning, err := storage.IsWarning(store, err); warning { s.instrumentOpts.Logger().Warn( "partial results: fanout to store returned warning", zap.Error(err), zap.String("store", store.Name()), zap.String("function", "SearchSeries")) continue } s.instrumentOpts.Logger().Warn( "fanout to store returned error", zap.Error(err), zap.String("store", store.Name()), zap.String("function", "SearchSeries")) return nil, err

robskillington · 2019-09-12T20:00:20Z

src/query/storage/fanout/storage.go

+					zap.String("function", "CompleteTags"))
+
+				return nil, err
+			}


Same thing as above, think you're missing a continue and may as well do the de-nesting "Go like" pattern/thing I typed up above.

robskillington · 2019-09-12T20:02:22Z

src/query/storage/types.go

+	BehaviorWarn
+	// BehaviorContainer is for storages that contain substorages. It is necessary
+	// to look at the returned error to determine if it's a failing error or
+	// a warning error.


Ah I see, maybe a link to this or just a brief duplicate-like explanation at the other definition would be good too.

codecov · 2019-09-12T21:45:59Z

Codecov Report

Merging #1938 into master will decrease coverage by 5%.
The diff coverage is 42.9%.

@@            Coverage Diff            @@
##           master    #1938     +/-   ##
=========================================
- Coverage    60.5%    55.5%   -5.1%     
=========================================
  Files        1105     1099      -6     
  Lines      104971   102648   -2323     
=========================================
- Hits        63604    57011   -6593     
- Misses      37102    41601   +4499     
+ Partials     4265     4036    -229

Flag	Coverage Δ
#aggregator	`69.1% <ø> (-10.7%)`	⬇️
#cluster	`44.7% <ø> (ø)`	⬆️
#collector	`47.9% <ø> (-15.8%)`	⬇️
#dbnode	`60.6% <ø> (-1.1%)`	⬇️
#m3em	`59.6% <ø> (ø)`	⬆️
#m3ninx	`60.9% <ø> (+2.1%)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.7% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (ø)`	⬆️
#query	`45.8% <42.9%> (-21.8%)`	⬇️
#x	`74.7% <ø> (+6.7%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80377b0...06a1075. Read the comment docs.

pavelnikolov · 2019-09-12T22:21:02Z

src/cmd/services/m3query/config/config.go


-// Remote is the configuration for a single remote host.
-type Remote struct {
+// RemoteConfiguration is the configuration for a single remote host.


nit: Why do you need this? It's already in the config package and in the config.go file. So you end up with config.RemoteConfiguration vs config.Remote. I prefer the shorter version.

Mostly to avoid conflicts with the new more concrete Remote type; ideally we shouldn't be using RemoteConfiguration anywhere outside this package, but we have to make it public for yaml parsing

pavelnikolov · 2019-09-12T22:23:39Z

src/cmd/services/m3query/config/remote_options.go

+)
+
+// Remote is an option for remote storage.
+type Remote struct {


nit: Why do you have both Remote and RemoteConfiguration?

So RemoteConfiguration is the parsed yaml, Remote is the parsed structure

pavelnikolov · 2019-09-12T22:39:21Z

src/cmd/services/m3query/config/config.go

@@ -450,6 +458,11 @@ type RPCConfiguration struct {
 	// RemoteListenAddresses will only allow for a single remote zone to be used.
 	RemoteListenAddresses []string `yaml:"remoteListenAddresses"`


Is RemoteListenAddresses deprecated in favour of Remotes?

Yeah; it used to limit to a single zone; with this PR, can technically do both but would recommend sticking to Remotes

At some point soon we'll update docs as well as our example configs to mirror this

robskillington

LGTM

pavelnikolov · 2019-09-13T05:16:27Z

src/cmd/services/m3query/config/config.go

+	// ErrorBehavior overrides the default error behavior for this host.
+	//
+	// NB: defaults to warning on error.
+	ErrorBehavior *storage.ErrorBehavior `yaml:"errorBehavior"`


If ErrorBehavior is uint8 how do you serialize it to yaml? Shouldn't you have some string representation instead of numbers? Also, I saw that the constants are using iota assignment which means that if in future someone adds more values, these need to always be appended at the end. Otherwise, existing apps might break.

So we've defined a custom yaml unmarshall here, which allows us to use the plaintext version of the desired behavior in the yaml. The documentation is definitely missing at the moment, but you can look at some of the new tests added in remote_options_test.go to see how the options will look in yaml (in all honesty you'd probably be fine with just taking the default approach which sets all remotes to BehaviorWarn)

[query] Remote storages can warn rather than error on failure

f309869

arnikola requested review from robskillington and benraskin92 September 12, 2019 18:51

Merge branch 'master' into arnikola/remote-error-behavior

ee3fe77

robskillington reviewed Sep 12, 2019

View reviewed changes

src/cmd/services/m3query/config/config.go Outdated Show resolved Hide resolved

robskillington reviewed Sep 12, 2019

View reviewed changes

src/query/server/server.go Outdated Show resolved Hide resolved

robskillington reviewed Sep 12, 2019

View reviewed changes

robskillington mentioned this pull request Sep 12, 2019

[query] Add a limit exceeded header if too many series are returned #1838

Merged

robskillington reviewed Sep 12, 2019

View reviewed changes

arnikola added 3 commits September 12, 2019 16:37

PR response

cb77526

Added fanout error tests and detection for remote Fetch path

6155d5e

Fix comment

06a1075

pavelnikolov reviewed Sep 12, 2019

View reviewed changes

robskillington approved these changes Sep 13, 2019

View reviewed changes

Merge branch 'master' into arnikola/remote-error-behavior

c0c80e1

arnikola merged commit 5898e6a into master Sep 13, 2019

arnikola deleted the arnikola/remote-error-behavior branch September 13, 2019 01:21

pavelnikolov reviewed Sep 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Remote storages can warn rather than error on failure #1938

[query] Remote storages can warn rather than error on failure #1938

arnikola commented Sep 12, 2019

robskillington Sep 12, 2019 •

edited

Loading

arnikola Sep 12, 2019

robskillington Sep 12, 2019

arnikola Sep 12, 2019

robskillington Sep 12, 2019 •

edited

Loading

arnikola Sep 12, 2019

arnikola Sep 12, 2019

robskillington Sep 12, 2019

arnikola Sep 12, 2019

robskillington Sep 13, 2019

robskillington Sep 12, 2019 •

edited

Loading

arnikola Sep 12, 2019

robskillington Sep 12, 2019

arnikola Sep 12, 2019

robskillington Sep 12, 2019

robskillington Sep 12, 2019

codecov bot commented Sep 12, 2019

pavelnikolov Sep 12, 2019 •

edited

Loading

arnikola Sep 12, 2019

pavelnikolov Sep 12, 2019 •

edited

Loading

arnikola Sep 12, 2019

pavelnikolov Sep 12, 2019

arnikola Sep 12, 2019

robskillington left a comment

pavelnikolov Sep 13, 2019

arnikola Sep 13, 2019

		@@ -450,6 +458,11 @@ type RPCConfiguration struct {
		// RemoteListenAddresses will only allow for a single remote zone to be used.
		RemoteListenAddresses []string `yaml:"remoteListenAddresses"`

[query] Remote storages can warn rather than error on failure #1938

[query] Remote storages can warn rather than error on failure #1938

Conversation

arnikola commented Sep 12, 2019

robskillington Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2019

Codecov Report

pavelnikolov Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelnikolov Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Sep 12, 2019 •

edited

Loading

robskillington Sep 12, 2019 •

edited

Loading

robskillington Sep 12, 2019 •

edited

Loading

pavelnikolov Sep 12, 2019 •

edited

Loading

pavelnikolov Sep 12, 2019 •

edited

Loading