Skip to content

fix(metrics): negative increment to a counter should not cause panics (NK-1729) #2492

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.

Sign up for GitHub

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

Merged
redbaron merged 4 commits into from
Apr 14, 2026
+56 −4
Merged

fix(metrics): negative increment to a counter should not cause panics (NK-1729) #2492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
495cc26
fix(metrics): negative increment to a counter should not cause panics
redbaron Apr 14, 2026
d0f6720
feat(metrics): derive wait time from metrics flush interval during tests
redbaron Apr 14, 2026
5e5511d
feat(metrics): track custom counter increments errors in the dedicated
redbaron Apr 14, 2026
28f2d06
feat(metrics): apply same cardinality restrictions as for custom metrics
redbaron Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ The format is based on [keep a changelog](http://keepachangelog.com) and this pr

## [Unreleased]

### Fixed
- Ignore negative custom runtime counter deltas to avoid panics during metrics collection.

## [3.38.0] - 2026-03-20
### Added
- Add runtime Satori client feature to delete identities.
Expand Down
4 changes: 4 additions & 0 deletions server/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,10 @@ func (m *LocalMetrics) StorageWriteRejectCount(tags map[string]string, delta int

// CustomCounter adds the given delta to a counter with the specified name and tags.
func (m *LocalMetrics) CustomCounter(name string, tags map[string]string, delta int64) {
if delta < 0 {
Comment on lines 527 to +529
Copy link
Copy Markdown
Member

@sesposito sesposito Apr 14, 2026

Choose a reason for hiding this comment

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

I was going to suggest something similar, do you think we should emit a warn log instead of silently dropping?

Copy link
Copy Markdown
Contributor Author

@redbaron redbaron Apr 14, 2026

Choose a reason for hiding this comment

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

I initially had a log, but then took it out, because counter increment can be a hot path and if it keeps happening then logs will be spammed.

If not log, then maybe counter_errors metric? this way there will be visibility into which counters failed to increment.

Copy link
Copy Markdown
Member

@sesposito sesposito Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Dedicated metric seems like the best option, perhaps custom_counter_negative_error?

Copy link
Copy Markdown
Contributor Author

@redbaron redbaron Apr 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

we can't use custom_ prefix for internal metric, because they are reserved for custom. Maybe:

metrics_collection_errors_count{name=custom_counter_abc,err=negative_increment} 1

Copy link
Copy Markdown
Member

@sesposito sesposito Apr 14, 2026

Choose a reason for hiding this comment

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

👍

return
}

scope := m.prometheusCustomScope
if len(tags) != 0 {
scope = scope.Tagged(tags)
Expand Down
39 changes: 39 additions & 0 deletions server/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2026 The Nakama Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package server

import (
"testing"
"time"

"go.uber.org/zap"
)

func TestMetricsCounterAddNegativeDoesNotPanics(t *testing.T) {
logger := zap.NewNop()
Copy link
Copy Markdown
Member

@sesposito sesposito Apr 14, 2026

Choose a reason for hiding this comment

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

Given that this test is time dependent and could potentially be flaky, should we t.Skip() it as part of the test suite?

Copy link
Copy Markdown
Contributor Author

@redbaron redbaron Apr 14, 2026

Choose a reason for hiding this comment

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

it reliably fails without a fix:

  • tally is configured with 1s flush interval
  • waits are 1.5 seconds to guarantee that flush has occurred

Copy link
Copy Markdown
Member

@sesposito sesposito Apr 14, 2026

Choose a reason for hiding this comment

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

I'm a bit weary of adding tests that depend on sleep as it'll increase the test execution time and it can pile up over time, although I'm aware sometimes it's unavoidable. If we want to keep it part of the suite, please infer the required sleep duration from the config value instead of hardcoding it.

Copy link
Copy Markdown
Contributor Author

@redbaron redbaron Apr 14, 2026

Choose a reason for hiding this comment

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

pushed change deriving sleeps from ReportingFreqSec.

it'll increase the test execution time and it can pile up over time

there is no tally.Flush() I could use even if I created private constructor for metrics collector. It also has to be above 1 second, because config option is with a second resolution.

cfg := NewConfig(logger)
cfg.Metrics.ReportingFreqSec = 1

metrics := NewLocalMetrics(logger, logger, nil, cfg)
defer metrics.Stop(logger)

module := &RuntimeGoNakamaModule{metrics: metrics}
module.MetricsCounterAdd("panic_counter", nil, 1)

time.Sleep(1500 * time.Millisecond)
module.MetricsCounterAdd("panic_counter", nil, -1)

time.Sleep(1500 * time.Millisecond)
}
Loading