Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

feat(node): Add maxCacheKeyLength to Redis integration (remove truncation) #18045

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.

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

Open
s1gr1d wants to merge 1 commit into develop
base: develop
Choose a base branch
Loading
from sig/truncation-redis

Conversation

@s1gr1d
Copy link
Member

@s1gr1d s1gr1d commented Oct 28, 2025

This removes the automatic truncation of the span description. Now, all cache keys are set in the description.

part of #17389

sentry-io[bot] reacted with hooray emoji
@s1gr1d s1gr1d added the Integration: redis Issues related to Redis support for the Sentry Node SDK label Oct 28, 2025
Comment on lines +34 to +50
describe('early returns', () => {
it.each([
{ desc: 'no args', cmd: 'get', args: [], response: 'test' },
{ desc: 'unsupported command', cmd: 'exists', args: ['key'], response: 'test' },
{ desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} },
{ desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } },
])('should always set sentry.origin but return early when $desc', ({ cmd, args, response, options = {} }) => {
vi.clearAllMocks();
Object.assign(_redisOptions, options);

cacheResponseHook(mockSpan, cmd, args, response);

expect(mockSpan.setAttribute).toHaveBeenCalledWith('sentry.origin', 'auto.db.otel.redis');
expect(mockSpan.setAttributes).not.toHaveBeenCalled();
expect(mockSpan.updateName).not.toHaveBeenCalled();
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I added some general tests for the handler, as there were none for this behavior so far.

span.updateName(truncate(spanDescription, 1024));
span.updateName(
_redisOptions.maxCacheKeyLength ? truncate(spanDescription, _redisOptions.maxCacheKeyLength) : spanDescription,
);
Copy link

@cursor cursor bot Oct 28, 2025

Choose a reason for hiding this comment

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

Bug: Redis Key Length Check Fails

The span.updateName logic uses a truthy check for _redisOptions.maxCacheKeyLength. When maxCacheKeyLength is 0, the span name isn't truncated, which is unexpected as it should become an empty string.

Fix in Cursor Fix in Web

Copy link
Contributor

node-overhead report 🧳

Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 9,067 - 11,574 -22%
GET With Sentry 1,359 15% 1,651 -18%
GET With Sentry (error only) 6,124 68% 7,686 -20%
POST Baseline 1,187 - 1,212 -2%
POST With Sentry 496 42% 561 -12%
POST With Sentry (error only) 1,051 89% 1,051 -
MYSQL Baseline 3,309 - 4,061 -19%
MYSQL With Sentry 443 13% 537 -18%
MYSQL With Sentry (error only) 2,662 80% 3,371 -21%

View base workflow run

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

Reviewers

@cursor cursor[bot] cursor[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Integration: redis Issues related to Redis support for the Sentry Node SDK

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /