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

mark disable_compat_1.9 to deprecate in 1.13, change default to true #12675

Merged
merged 6 commits into from
Apr 1, 2022

Conversation

acpana
Copy link
Contributor

@acpana acpana commented Mar 31, 2022

overview

I've been sitting on this patch for a bit. IIUC, the rationale was to remove 1.9 style consul.http... metrics once 1.9 stopped being supported. But I don't think we can do that "smoothly" for 1.12 just yet.

Instead of completely getting rid of them and throwing anyone that still instruments over these metrics into a panic, this PR suggests modifying the behavior of disable_compat_1.9 flag to default to true. Yes, folks will still need to update their config but at least the metrics will still exist until 1.13 to give time to use the new consul.api.http... introduced in 1.10.

todo:

  • changelog

Signed-off-by: FFMMM FFMMM@users.noreply.github.com

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
@acpana acpana requested a review from a team as a code owner March 31, 2022 20:09
@github-actions github-actions bot added theme/config Relating to Consul Agent configuration, including reloading type/docs Documentation needs to be created/updated/clarified labels Mar 31, 2022
@hashicorp-ci
Copy link
Contributor

🤔 This PR has changes in the website/ directory but does not have a type/docs-cherrypick label. If the changes are for the next version, this can be ignored. If they are updates to current docs, attach the label to auto cherrypick to the stable-website branch after merging.

Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – consul March 31, 2022 20:12 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging March 31, 2022 20:12 Inactive
@rboyer
Copy link
Member

rboyer commented Mar 31, 2022

You should add a point about the upgrade backwards incompatibility here: https://www.consul.io/docs/upgrading/upgrade-specific

.changelog/12675.txt Outdated Show resolved Hide resolved
@rboyer rboyer added this to the 1.12.0-beta1 milestone Mar 31, 2022
Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – consul March 31, 2022 20:18 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging March 31, 2022 20:18 Inactive
Signed-off-by: FFMMM <FFMMM@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging March 31, 2022 21:53 Inactive
@vercel vercel bot temporarily deployed to Preview – consul March 31, 2022 21:53 Inactive
@acpana acpana requested a review from rboyer March 31, 2022 21:54
Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

A few minor wording changes but otherwise looks good

assertMetricNotExists(t, respRec, "agent_http_http_GET_v1_agent_members")
})

t.Run("check that we cam still turn on consul.http metrics", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cam => can

@@ -2175,7 +2175,7 @@ There are also a number of common configuration options supported by all provide
geo location or datacenter, dc:sfo). By default, this is left blank and not used.

- `disable_compat_1.9` ((#telemetry-disable_compat_1.9))
This allows users to disable metrics deprecated in 1.9 so they are no longer emitted, saving on performance and storage in large deployments. Defaults to false.
This allows users to disable metrics deprecated in 1.9 so they are no longer emitted, saving on performance and storage in large deployments. As of 1.12 this defaults to `true` and will be removed, along with 1.9 style http metrics in 1.13.
Copy link
Contributor

Choose a reason for hiding this comment

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

'saving on performance' is odd, maybe improving performance and reducing storage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good! That sentence was there before me but I can try to reword it.

agent/metrics_test.go Outdated Show resolved Hide resolved
@acpana
Copy link
Contributor Author

acpana commented Apr 1, 2022

all test-integrations workflows actually succeeded in the previous run:

running into GH >< circle reporting issues again; PR approved beforehand, merging in!

@acpana acpana merged commit 973d2d0 into main Apr 1, 2022
@acpana acpana deleted the ffmmm/no-19-metrics branch April 1, 2022 17:35
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/621636.

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 type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants