Skip to content

feat(grouper): add counters to the grouper worker#521

Open
e11sy wants to merge 9 commits intomasterfrom
imp/grouper-increment-redis-counters
Open

feat(grouper): add counters to the grouper worker#521
e11sy wants to merge 9 commits intomasterfrom
imp/grouper-increment-redis-counters

Conversation

@e11sy
Copy link
Contributor

@e11sy e11sy commented Feb 7, 2026

Problem

This is the part of the work on sentry counters issue

Sentry events trigger counters (chart) increments and then are silently dismissed in sentry worker

this leads to invalid charts behaviour

Solution

Move charts update to the grouper to guarantee that we increment counters after the moment, when event is stored in the db

Moved charts redis time series logic to grouper RedisHelper

Note

Make sure to check this collector PR also

@codex-assistant
Copy link

Please add a PR description 🙂

Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference.

The PR was moved to Draft until a description is added.

@codex-assistant codex-assistant bot marked this pull request as draft February 7, 2026 14:43
@e11sy e11sy force-pushed the imp/grouper-increment-redis-counters branch from e374a3f to 8777e77 Compare February 7, 2026 14:44
@codex-assistant
Copy link

Please add a PR description 🙂

Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference.

The PR was moved to Draft until a description is added.

@codex-assistant
Copy link

Please add a PR description 🙂

Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference.

The PR was moved to Draft until a description is added.

@codex-assistant
Copy link

Please add a PR description 🙂

Respect the reviewers — a description helps others understand the changes and review them faster. Keep it short, clear, and to the point. It also serves as documentation for future reference.

The PR was moved to Draft until a description is added.

@codex-assistant
Copy link

Thanks for adding a description — the PR is now marked as Ready for Review.

@codex-assistant codex-assistant bot marked this pull request as ready for review February 7, 2026 15:08
n0str
n0str previously approved these changes Feb 8, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR moves “events accepted” counter updates into the Grouper worker so project charts are updated only after an event is persisted, and introduces RedisTimeSeries helper methods to support that flow.

Changes:

  • Add RedisTimeSeries helper methods (TS.CREATE, TS.ADD, TS.INCRBY) with “safe” wrappers that create series on-demand.
  • Record events-accepted metrics (minutely/hourly/daily) at the end of GrouperWorker.handle.
  • Add tests asserting the three writes happen, failures are logged without breaking processing, and metrics are recorded once per event.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
workers/grouper/tests/index.test.ts Adds tests validating metrics writes and failure behavior.
workers/grouper/src/redisHelper.ts Adds RedisTimeSeries command helpers and safe wrappers.
workers/grouper/src/index.ts Records project metrics (3 granularities) after event handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +264 to +271
private buildLabelArguments(labels: Record<string, string>): string[] {
const labelArgs: string[] = [ 'LABELS' ];

for (const [labelKey, labelValue] of Object.entries(labels)) {
labelArgs.push(labelKey, labelValue);
}

return labelArgs;
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

buildLabelArguments always returns ['LABELS'] even when labels is empty. Because tsAdd/tsIncrBy default labels = {}, this can generate RedisTimeSeries commands ending with a bare LABELS token, which Redis will reject as a wrong-arity command. Consider returning an empty array when there are no labels (or make labels required for these public methods).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we never pass empty labels object


args.push(...this.buildLabelArguments(labels));

await this.redisClient.sendCommand(args);
Copy link

Copilot AI Feb 8, 2026

Choose a reason for hiding this comment

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

tsCreateIfNotExists does a separate EXISTS check and then TS.CREATE. Under concurrency, two workers can both observe the key as missing and then one of the TS.CREATE calls will fail with "key already exists", which currently bubbles up and causes the metric write to be dropped. Consider handling the "already exists" error from TS.CREATE (treat as success) or using an atomic pattern so concurrent creation is safe.

Suggested change
await this.redisClient.sendCommand(args);
try {
await this.redisClient.sendCommand(args);
} catch (error) {
/**
* Handle race condition where another worker created the key between
* the EXISTS check and TS.CREATE. In that case, RedisTimeSeries will
* return an error like "ERR TSDB: key already exists". We can safely
* treat this as success.
*/
if (
error instanceof Error &&
/key already exists/i.test(error.message)
) {
return;
}
throw error;
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rewritten to lua

e11sy and others added 2 commits February 8, 2026 21:44
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

3 participants