Don't expire all time total counts

What does this MR do and why?

Fixes a bug: Total count metrics without time frame should never expire. It is important to get this merged as soon as possible so we don't loose any total counters.

We might also want to go through and remove the expiry date for all our Redis keys that doesn't have a a ´<year_week>` component.

We should also add a test to make sure this doesn't happen in the future again.

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

Before	After

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

assigned to @j_lar

added backend label

	2 Warnings
	This merge request does not refer to an existing milestone.
	You've made some app changes, but didn't add any tests. That's OK as long as you're refactoring existing code, but please consider adding any of the maintenancepipelines, maintenancerefactor, maintenanceworkflow, documentation, QA labels.

	1 Message
	CHANGELOG missing: If this merge request needs a changelog entry, add the Changelog trailer to the commit message you want to add to the changelog. If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Category	Reviewer	Maintainer
backend	@jhyson (UTC+12, 10 hours ahead of author)	@drew (UTC+0, 2 hours behind author)

Please check reviewer's status!

• Reviewer is available!
• Reviewer is unavailable!

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added analytics instrumentation label

added typebug label

added severity1 label

@gitlab-org/analytics-section/analytics-instrumentation/engineers @bastirehm

I realized I made a bad rebase when I read Piotr's update: Added expiration date to Redis keys for total counters.

I'm OOO the next days. Can you please make sure this is merged?

We need this merged before the end of the milestone so we don't ship a version without. For gitlab.com we have to go back end remove the expiry from the relevant total count redis keys.

@j_lar Thanks for catching this, this needs some changes to specs, so I'll take it over!

I agree this should get into 17.0. This won't cause problems with existing total counters, but new ones (for new total metrics) will have the expiry for both weekly and (mistakenly) all time frame.

@j_lar @bastirehm This is already outdated as !151646 (merged) got reverted. See this comment: !151646 (comment 1898699480)

Not sure if any Redis keys are affected during the brief time this was deployed. Probably not, but we might still want to check.

@pskorupa, @j_lar, do I understand correctly that in this case an expiry date would've been added to the keys, e.g. 60 days into the future? What kind of access do we need to be able to check whether this happened?

@bastirehm Yes, 6 weeks, but applied only to newly-created keys (if any were even added). You'd have to do a TTL command a particular key to know, so some sort of access to Redis is required. I don't think our Teleport supports Redis console, but maybe we could just do it through Rails console? WDYT

@bastirehm @j_lar I checked the expiration logic I wrote, and it applies an expiration timeout to a key if it doesn't already have one. So we'll have to assume that every total count key has an expiration timeout now and remove all of them.

We could achieve that by running a PERSIST command on every affected key.

@pskorupa could we do that as a separate MR in a one-off migration that just runs through all-time count keys?

@bastirehm You mean a background migration? That could work, but I haven't heard about using them to do stuff outside relational database changes. If we could get some more straightforward access to Redis to just run a bunch of arbitrary command, that would be the best, but I don't know if that's possible.

@pskorupa I mean a redis migration, the same as when we migrate keys. Afaik this is run as a post-deploy migration. The advantage I see with this would be that it's a reviewed code-change.

@pskorupa @bastirehm I agree that we should make post migration to make sure it is executes in all environments.

I made this redis migration a while ago.

Can we scan for keys starting with USAGE_ and not ending with -<year>-<week> and update those? I don't know if we can call PERSIST through the Ruby wrapper though.

@bastirehm @j_lar Ah, of course. Yeah, a post-deployment migration should work.

Can we scan for keys starting with USAGE_

Not really - some of the keys don't have this prefix, specifically the ones that have include_usage_prefix: false option set in their metric definition. We have to dump a list of Redis keys from total counter metrics at the time the change was deployed + grab the rest from lib/gitlab/usage_data_counters/total_counter_redis_key_overrides.yml for the metrics that were migrated to internal events.

I don't know if we can call PERSIST through the Ruby wrapper though.

I don't see why not, here are the docs.

@j_lar I created a separate issue #461381 (closed) to keep track of this and assigned it to you.

requested review from @pskorupa

assigned to @pskorupa

added groupanalytics instrumentation label

changed milestone to %17.0

added devopsmonitor sectionanalytics labels

removed review request for @pskorupa

mentioned in issue #461381 (closed)

mentioned in merge request !152778 (merged)

closed

mentioned in commit e80f0f77

mentioned in merge request !153095 (merged)

mentioned in commit a95839ed

mentioned in commit a3e7e210