feat: deprecations metric #13735

Joibel · 2024-10-10T13:15:36Z

A metric to measure the use of deprecated features in the code base

Motivation

We have a number of deprecated features.

As a user it will be hard to know if it is safe to upgrade beyond the point when the deprecated feature becomes removed.

In preparation for removing the features this metric can be monitored to ensure you're good to upgrade.

Modifications

There is a deliberate design decision to ensure the counting was simple. It would be worse for us to have a bug in the counting code which meant we didn't flag up a use when there was one. So each deprecated field is flagged at point of use by the code base.

This has problems:

We may not care in this case - for example the validator called from the CLI doesn't have telemetry, so this doesn't work here.
We will look at some fields multiple times in a single run of the reconciliation loop, so the number will go up faster than once per workflow run. I considered that attempting to get 'once per' behaviour would introduce more bugs, and this provides the necessary information

There are two new modules:

context

This is a general purpose module for reading and writing object metadata into context which is the canonical way of passing data around. This avoids us having to expose too much information in the wrong places.

deprecation

This is a separate module with no imports except context above which can be called from anywhere without anything other than a context. The context does not need to contain the desired information.

It holds an evil global variable specifying what to do when a deprecation is flagged. When this has been initialised by the controller to point to the metrics function which records deprecations we get the metric number going up. Otherwise it's silent.

If the namespace is available in context we report it. This will be the namespace of the running workflow or cronworkflow.

Overall

This means this modifies a lot of code paths to pass context, but I consider this a reasonable thing that we should want in general anyway.

NOT Implemented here

The validator does not flag up deprecated features. It should so I've raised #13736

We could log these too, including name which might help in hunting down stragglers. (Some of these come from WorkflowTemplates, so it's not immediately going to point the finger).

Verification

Unit tests have been added. e2e test for each deprecation added.

isubasinghe

Minor comment, will leave up to you to decide.

isubasinghe · 2024-10-11T05:12:38Z

util/context/context.go

+func ObjectName(ctx context.Context) string {
+	if n, ok := ctx.Value(name).(string); ok {
+		return n
+	}
+	return ""
+}


I'd personally prefer returning a boolean as well.
Exactly like how maps work.

I know this is guaranteed to have this key though.

But we do check the ok in the cast, continuing on with this lack of assuming things, we should also return an ok bool.

When someone needs it we can add that, right now it wouldn't be used, so not adding in this commit.

isubasinghe · 2024-10-11T05:12:43Z

util/context/context.go

+func ObjectNamespace(ctx context.Context) string {
+	if n, ok := ctx.Value(namespace).(string); ok {
+		return n
+	}


A metric to measure the use of deprecated features in the code base Signed-off-by: Alan Clucas <[email protected]>

agilgur5

Had some WIP comments here too

agilgur5 · 2024-10-11T03:26:14Z

mkdocs.yml

@@ -248,6 +248,7 @@ nav:
          - offloading-large-workflows.md
          - workflow-archive.md
          - metrics.md
+          - deprecations.md


I'd put this next to upgrading.md and link to it from there too.

Same as the "New features" page we discussed in the Contributor Meeting, having it per version and matching up with the upgrading guide will make it easier to follow. EDIT: See also #13739 (comment)

Although arguably deprecations should be part of the upgrading guide.

Although arguably deprecations should be part of the upgrading guide.

I see this has been done in #13757 now

agilgur5 · 2024-10-18T04:25:26Z

docs/deprecations.md

Similarly to above, I would put all the H2 headings here a under a 3.6 version heading so that the upgrading guide could reference it per version. With no versions on this page, it's hard to understand when something was deprecated, and some of these also reference features that are only available in newer versions

As we have already had users attempt to use 3.5 and 3.6 features in older versions when there were no version annotations (#13645 as one example), I could definitely see users reading this, seeing the deprecation, trying the new syntax, and then getting a syntax error and raising an issue.

agilgur5 · 2024-10-18T04:29:14Z

docs/deprecations.md

+Sometimes a feature of Argo Workflows is deprecated.
+You should stop using a deprecated feature as it may be removed in a future minor or major release of Argo Workflows.
+
+To determine if you are using a deprecated feature the [`deprecated_feature`](metrics.md#deprecated_feature) metric can help.


It would definitely be good to have a version annotation for this metric too, as people will see this page, try to use it, and not realize that it's not available in older versions, and perhaps even think they're in the clear because of that.

agilgur5 · 2024-10-18T04:32:20Z

docs/deprecations.md

+If the number is going up the feature is still in use by your system.
+If the metric is not present or no longer increasing are no longer using the monitored deprecated features.
+
+## `cronworkflow schedule`


I would name these for their respective features so that they are easily correlated, and then perhaps have a "> cronworkflow schedule attribute" to indicate that this is what the metric displays. I think it's difficult to tell that this is referring to those specific names unless you have specifically came from the metric's page and read it in detail.

This page isn't specific to the metric though, it's just named "Deprecations" in general as well, so it can reference it but should be its own independent page as well

It would also be good to link to the feature page for each of these which will have more details and examples

agilgur5 · 2024-10-18T04:33:52Z

docs/deprecations.md

+  schedule: "30 1 * * *"
+```
+
+is replaced with
+
+```yaml
+spec:
+  schedules:
+    - "30 1 * * *"


These examples would likely be easier to understand quickly with a ```diff, i.e.:

- schedule: "30 1 * * *" + schedules: + - "30 1 * * *"

agilgur5 · 2024-10-18T04:36:15Z

docs/deprecations.md

+## `cronworkflow schedule`
+
+The spec field `schedule` which takes a single value is replaced by `schedules` which takes a list.
+To update this replace the `schedule` with `schedules` as in the following example


Suggested change

To update this replace the `schedule` with `schedules` as in the following example

To update this replace the `schedule` with `schedules` as in the following example:

Colons are also missing in prior to all the examples, which are conventionally (and gramatically) used when describing an example in the docs

Joibel added the area/metrics label Oct 10, 2024

Joibel mentioned this pull request Oct 10, 2024

Validator should flag deprecated features #13736

Open

Joibel force-pushed the deprecation-metric branch 2 times, most recently from ba4f1a1 to b0af5bb Compare October 10, 2024 13:26

Joibel marked this pull request as ready for review October 10, 2024 15:34

isubasinghe approved these changes Oct 11, 2024

View reviewed changes

Joibel force-pushed the deprecation-metric branch from c13c231 to 25718e9 Compare October 11, 2024 07:23

feat: deprecations metric

37a371a

A metric to measure the use of deprecated features in the code base Signed-off-by: Alan Clucas <[email protected]>

Joibel force-pushed the deprecation-metric branch from 25718e9 to 37a371a Compare October 11, 2024 07:47

Joibel merged commit 91765fa into argoproj:main Oct 11, 2024
28 checks passed

Joibel deleted the deprecation-metric branch October 11, 2024 09:26

agilgur5 added this to the v3.6.0 milestone Oct 11, 2024

This was referenced Oct 15, 2024

docs: add deprecations to upgrading guide #13757

Merged

REQUEST: Self-nomination for sub-project lead for @Joibel argoproj/argoproj#330

Open

agilgur5 mentioned this pull request Oct 18, 2024

docs: new features in 3.6 #13739

Open

agilgur5 reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deprecations metric #13735

feat: deprecations metric #13735

Joibel commented Oct 10, 2024 •

edited

Loading

isubasinghe left a comment

isubasinghe Oct 11, 2024

Joibel Oct 11, 2024

isubasinghe Oct 11, 2024

agilgur5 left a comment

agilgur5 Oct 11, 2024 •

edited

Loading

agilgur5 Oct 18, 2024

agilgur5 Oct 18, 2024 •

edited

Loading

agilgur5 Oct 18, 2024

agilgur5 Oct 18, 2024

agilgur5 Oct 18, 2024

agilgur5 Oct 18, 2024

agilgur5 Oct 18, 2024

	To update this replace the `schedule` with `schedules` as in the following example
	To update this replace the `schedule` with `schedules` as in the following example:

feat: deprecations metric #13735

feat: deprecations metric #13735

Conversation

Joibel commented Oct 10, 2024 • edited Loading

Motivation

Modifications

context

deprecation

Overall

NOT Implemented here

Verification

isubasinghe left a 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

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agilgur5 Oct 18, 2024 • 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

Joibel commented Oct 10, 2024 •

edited

Loading

agilgur5 Oct 11, 2024 •

edited

Loading

agilgur5 Oct 18, 2024 •

edited

Loading