Skip to content

Conversation

adbista
Copy link
Contributor

@adbista adbista commented Jul 22, 2025

Which problem is this PR solving?

Short description of the changes

  • Add support for Redis v5.
  • Replaced deprecated semantic attributes with current semantic conventions
  • Updated tests

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

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

The version number in README.md needs updating as well 🙏

@adbista adbista requested a review from seemk July 23, 2025 18:06
@adbista adbista force-pushed the feat/instrumentation-redis/redis5-support branch from 5f502a6 to 494563b Compare August 1, 2025 11:08
Copy link

codecov bot commented Aug 1, 2025

Codecov Report

❌ Patch coverage is 67.27273% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.52%. Comparing base (5836d7a) to head (033c4dd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...instrumentation-redis/src/v2-v3/instrumentation.ts 34.78% 15 Missing ⚠️
packages/instrumentation-redis/src/redis.ts 66.66% 2 Missing ⚠️
...instrumentation-redis/src/v4-v5/instrumentation.ts 94.44% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (67.27%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
- Coverage   89.81%   89.52%   -0.29%     
==========================================
  Files         192      192              
  Lines        9638     9677      +39     
  Branches     1988     2009      +21     
==========================================
+ Hits         8656     8663       +7     
- Misses        982     1014      +32     
Files with missing lines Coverage Δ
packages/instrumentation-redis/src/v4-v5/utils.ts 100.00% <100.00%> (ø)
...instrumentation-redis/src/v4-v5/instrumentation.ts 72.98% <94.44%> (ø)
packages/instrumentation-redis/src/redis.ts 76.92% <66.66%> (ø)
...instrumentation-redis/src/v2-v3/instrumentation.ts 21.11% <34.78%> (+3.96%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@moekify
Copy link

moekify commented Aug 4, 2025

Would be great to get this merged :)

@dyladan
Copy link
Member

dyladan commented Aug 6, 2025

@blumamir any chance you can get this on your review queue?

dyladan
dyladan previously requested changes Aug 6, 2025
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks like this does not follow the guidance at https://github.com/open-telemetry/semantic-conventions/tree/main/docs/database

Specifically, the environment variable opt-in mechanism for new semconv breaking changes

@adbista adbista requested a review from dyladan August 11, 2025 10:38
@dyladan dyladan dismissed their stale review August 13, 2025 16:49

Looks like ENV var stuff is addressed

@ekarademir
Copy link

ekarademir commented Aug 15, 2025

We are all rooting for this PR to be merged soon 😅

@adbista
Copy link
Contributor Author

adbista commented Aug 20, 2025

I think the coverage issue arises because different Redis versions (v2-v3 and v4-v5) use separate code paths and tests for each version are run independently (v2-v3 and v4-v5 have distinct sets of tests) and this can lead to incorrect coverage report, which is why the codecov/patch failed.

@trentm
Copy link
Contributor

trentm commented Aug 20, 2025

I think the coverage issue arises because different Redis versions (v2-v3 and v4-v5) use separate code paths ...

Yup, exactly. Don't worry about the codecov failing check. Part of the coming work in #2866 will change to getting coverage data for multiple target module versions using the test-all-versions tests rather than from just npm test for a single version.

@adbista
Copy link
Contributor Author

adbista commented Aug 25, 2025

@dyladan @blumamir, is there anything else I should do for this pull request, or does everything look good?

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thank you for working on this.

Added few nits which can be nice to address (but not blocker for merge)

@blumamir blumamir merged commit 8b09de9 into open-telemetry:main Aug 27, 2025
24 of 25 checks passed
@dyladan dyladan mentioned this pull request Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support redis v5
7 participants