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

Add flag for disabling 1.9 metrics backwards compatibility and warnings when set to default #8877

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

mkcp
Copy link
Contributor

@mkcp mkcp commented Oct 8, 2020

This PR is a follow up to #8271 to enable consul.api.http by default and give users a flag to disable metrics under consul.http.... We provide a deprecation warning if users are emitting the deprecated metrics.

Adding a Consul Config Field

This is a checklist of all the places you need to update when adding a new field
to config. There may be a few other special cases not included but this covers
the majority of configs.

We suggest you copy the raw markdown into a gist or local file and check them
off as you go (you can mark them as done by replace [ ] with [x] so github
renders them as checked). Then please include the completed lists you worked
through in your PR description
.

Examples of special cases this doesn't cover are:

  • If the config needs special treatment like a different default in -dev mode
    or differences between OSS and Enterprise.
  • If custom logic is needed to support backwards compatibility when changing
    syntax or semantics of anything

There are four specific cases covered with increasing complexity:

  1. adding a simple config field only used by client agents
  2. adding a CLI flag to mirror that config field
  3. adding a config field that needs to be used in Consul servers
  4. adding a field to the Service Definition

Adding a Simple Config Field for Client Agents

  • Add the field to the Config struct (or an appropriate sub-struct) in
    agent/config/config.go.
  • Add the field to the actual RuntimeConfig struct in
    agent/config/runtime.go.
  • Add an appropriate parser/setter in agent/config/builder.go to
    translate.
  • Add the new field with a random value to both the JSON and HCL blobs in
    TestFullConfig in agent/config/runtime_test.go, it should fail now, then
    add the same random value to the expected struct in that test so it passes
    again.
  • Add the new field and it's default value to TestSanitize in the same
    file. (Running the test first gives you a nice diff which can save working
    out where etc.)
  • If your new config field needed some validation as it's only valid in
    some cases or with some values (often true).
    • Add validation to Validate in agent/config/builder.go.
    • Add a test case to the table test TestConfigFlagsAndEdgeCases in
      agent/config/runtime_test.go.
  • If your new config field needs a non-zero-value default.
    • Add that to DefaultSource in agent/config/defaults.go.
    • Add a test case to the table test TestConfigFlagsAndEdgeCases in
      agent/config/runtime_test.go.
  • If your config should take effect on a reload/HUP.
    • Add necessary code to to trigger a safe (locked or atomic) update to
      any state the feature needs changing. This needs to be added to one or
      more of the following places:
      • ReloadConfig in agent/agent.go if it needs to affect the local
        client state or another client agent component.
      • ReloadConfig in agent/consul/client.go if it needs to affect
        state for client agent's RPC client.
    • Add a test to agent/agent_test.go similar to others with prefix
      TestAgent_reloadConfig*.
  • Add documentation to website/source/docs/agent/options.html.md.

Done! You can now use your new field in a client agent by accessing
s.agent.Config.<FieldName>.

@mkcp mkcp requested a review from a team October 8, 2020 00:18
@github-actions github-actions bot added theme/config Relating to Consul Agent configuration, including reloading theme/internals Serf, Raft, SWIM, Lifeguard, Anti-Entropy, locking topics type/docs Documentation needs to be created/updated/clarified type/docs-cherrypick labels Oct 8, 2020
@mkcp mkcp added the theme/telemetry Anything related to telemetry or observability label Oct 8, 2020
@@ -7379,6 +7382,7 @@ func TestSanitize(t *testing.T) {
"CirconusSubmissionInterval": "",
"CirconusSubmissionURL": "",
"Disable": false,
"DisableCompatOneNine": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would lean towards renaming this flag to something more metric... focused?
DisableMetricsCompat may be good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This config is contained within the telemetry block, so you have to configure it as telemetry { disable_compat_1.9 = true } if you want to set it. I believe this does a good job of scoping it to mean "only disables metrics" even if it looks a bit sketch in isolation.

Make `consul.http...` have an ellipsis rather than just two dots to denote that there's a lot of different metrics under that path.
@mkcp mkcp merged commit adeabf2 into master Oct 8, 2020
@mkcp mkcp deleted the mkcp/telemetry/consul.api.http branch October 8, 2020 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/config Relating to Consul Agent configuration, including reloading theme/internals Serf, Raft, SWIM, Lifeguard, Anti-Entropy, locking topics theme/telemetry Anything related to telemetry or observability type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants