Skip to content

build: Bump taskbroker-client to 0.1.8#112639

Closed
gricha wants to merge 3 commits intomasterfrom
gricha/build/bump-taskbroker-client
Closed

build: Bump taskbroker-client to 0.1.8#112639
gricha wants to merge 3 commits intomasterfrom
gricha/build/bump-taskbroker-client

Conversation

@gricha
Copy link
Copy Markdown
Member

@gricha gricha commented Apr 9, 2026

Bumps taskbroker-client from 0.1.7 to 0.1.8 to pick up latest changes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 9, 2026
@gricha gricha marked this pull request as ready for review April 9, 2026 23:16
@gricha gricha requested a review from a team as a code owner April 9, 2026 23:16
taskbroker-client 0.1.8 added gauge as an abstract method on
MetricsBackend. Implement it to match the existing pattern.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd3bb67. Configure here.

value: float,
instance: str | None = None,
tags: Tags | None = None,
sample_rate: float = 1,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gauge bypasses global metrics sample rate setting

Low Severity

The new gauge method defaults sample_rate to 1, unlike incr, distribution, and timer which all default to None and fall back to settings.SENTRY_METRICS_SAMPLE_RATE. In deployments that override SENTRY_METRICS_SAMPLE_RATE to a value other than 1.0, gauge metrics will always be emitted at 100% while all other metric types respect the configured rate.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit dd3bb67. Configure here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a valid bug.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The method definition in taskbroker-client also has sample_rate defaulted to 1.

sentry_metrics.gauge(
key,
value,
instance=instance,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: The gauge method's sample_rate defaults to 1, unlike other metrics methods which respect the global SENTRY_METRICS_SAMPLE_RATE setting.
Severity: MEDIUM

Suggested Fix

Change the signature of the gauge method to sample_rate: float | None = None. Inside the method, apply the global settings.SENTRY_METRICS_SAMPLE_RATE if sample_rate is None, mirroring the implementation of the incr, distribution, and timer methods in the same class.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/taskworker/adapters.py#L75

Potential issue: The `gauge` method in `SentryMetricsBackend` is defined with a default
`sample_rate` of `1`. This is inconsistent with other methods in the same class (`incr`,
`distribution`, `timer`), which accept `None` and then apply the global
`settings.SENTRY_METRICS_SAMPLE_RATE`. As a result, any calls to `gauge` from the
`taskbroker-client` library that do not explicitly provide a `sample_rate` will bypass
the global configuration and always sample at 100%. This could lead to an unexpected
increase in metric volume if a lower global sample rate is configured.

Did we get this right? 👍 / 👎 to inform future reviews.

@markstory
Copy link
Copy Markdown
Member

There is another pull request #112629 which is also going to update the taskbroker-client library, we could go with that one as it contains other changes as well.

@gricha
Copy link
Copy Markdown
Member Author

gricha commented Apr 10, 2026

Yes of course, I just wanted to make sure it's there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants