Skip to content
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

[matcher] [coordinator] Add RequireNamespaceWatchOnInit option #3468

Merged
merged 2 commits into from
May 6, 2021

Conversation

wesleyk
Copy link
Collaborator

@wesleyk wesleyk commented May 5, 2021

This option on the matcher will require that namespace
values and corresponding rulesets are loaded
in the matcher as part of startup.

This is useful in ensuring dynamic rules are loaded
before we start ingesting metrics.

What this PR does / why we need it:

Fixes #

Special notes for your reviewer:

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


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


@@ -302,6 +315,13 @@ func (n *namespaces) process(value interface{}) error {
n.log.Error("failed to watch ruleset updates",
zap.String("ruleSetKey", ruleSet.Key()),
zap.Error(err))

// Track errors if we explicitly want to ensure watches succeed.
if n.requireNamespaceWatchOnInit {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one awkward bit here is that this also covers the update case, not just the init case. However even if we propagate an error in that case, in the update path it gets swallowed regardless

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #3468 (df9bb93) into master (df9bb93) will not change coverage.
The diff coverage is n/a.

❗ Current head df9bb93 differs from pull request most recent head 664a90a. Consider uploading reports for the commit 664a90a to get more accurate results

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3468   +/-   ##
======================================
  Coverage    55.9%   55.9%           
======================================
  Files         548     548           
  Lines       61280   61280           
======================================
  Hits        34294   34294           
  Misses      23902   23902           
  Partials     3084    3084           
Flag Coverage Δ
aggregator 57.3% <0.0%> (ø)
cluster ∅ <0.0%> (∅)
collector 54.3% <0.0%> (ø)
dbnode 60.3% <0.0%> (ø)
m3em 46.4% <0.0%> (ø)
metrics 20.7% <0.0%> (ø)
msg 74.5% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 df9bb93...664a90a. Read the comment docs.

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but can we make the coordinator by default set this to true?

@wesleyk wesleyk enabled auto-merge (squash) May 6, 2021 03:11
@wesleyk wesleyk disabled auto-merge May 6, 2021 04:09
This option on the matcher will require that namespace
values and corresponding rulesets are loaded
in the matcher as part of startup.

This is useful in ensuring dynamic rules are loaded
before we start ingesting metrics.
@wesleyk
Copy link
Collaborator Author

wesleyk commented May 6, 2021

Need to default this to false since values won't always be set

@wesleyk wesleyk enabled auto-merge (squash) May 6, 2021 20:45
@wesleyk wesleyk merged commit b770ecf into master May 6, 2021
@wesleyk wesleyk deleted the wesley-require branch May 6, 2021 21:02
soundvibe added a commit that referenced this pull request May 10, 2021
* master:
  [query] Add Graphite movingWindow() function (#3484)
  [lint] Add missing build tags to linter configuration (#3480)
  [query] Fix Graphite nil arg not interpreted as explicit nil (#3481)
  [query] Fix Graphite context time window expansions not being cumulative
  [readme] Remove coverage badge due to CodeCov not able to execute (#3476)
  [query] Fix using median with aggregateWithWildcards and support more aggregate functions (#3469)
  [matcher] [coordinator] Add RequireNamespaceWatchOnInit option (#3468)
  [buildkite] Remove codecov uploading from unit tests (#3475)
  [aggregator] Add integration test for aggregator placement changes (#3465)
  [tools] Use streaming reads in read_data_files (#3474)
  [tools] Close the reader in read_data_files (#3473)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants