Skip to content
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

fix(instrumentation-redis-4): avoid crash from incorrect this._diag ref #2397

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Aug 22, 2024

Fixes: #2389

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.52%. Comparing base (dfb2dff) to head (32dd112).
Report is 218 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2397      +/-   ##
==========================================
- Coverage   90.97%   90.52%   -0.45%     
==========================================
  Files         146      157      +11     
  Lines        7492     7622     +130     
  Branches     1502     1571      +69     
==========================================
+ Hits         6816     6900      +84     
- Misses        676      722      +46     
Files Coverage Δ
...try-instrumentation-redis-4/src/instrumentation.ts 81.40% <100.00%> (+1.30%) ⬆️

... and 74 files with indirect coverage changes

@trentm trentm merged commit de7a6cb into open-telemetry:main Aug 22, 2024
21 checks passed
@trentm trentm deleted the tm-fix-instr-redis-this-diag-crash branch August 22, 2024 15:43
@dyladan dyladan mentioned this pull request Aug 22, 2024
trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Aug 22, 2024
…lient URL is the empty string

Issue open-telemetry#2389 showed that this instrumentation would *crash* on a Redis
client configured with `{url: ''}` (an empty string). The crash was
fixed in open-telemetry#2397. This change avoids the once-crashy code, and hence
the diag.error spam, by using the same guard before attempting to parse
the configured client URL that the Redis client itself does, `if
(options?.url)`,

Refs: open-telemetry#2389
pichlermarc pushed a commit that referenced this pull request Sep 4, 2024
…lient URL is the empty string (#2399)

Issue #2389 showed that this instrumentation would crash on a Redis
client configured with {url: ''} (an empty string). The crash was
fixed in #2397. This change avoids the once-crashy code, and hence
the diag.error spam, by using the same guard before attempting to parse
the configured client URL that the Redis client itself does, if (options?.url),

Refs: #2389
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.

App crashes with a TypeError when using the Redis instrumentation
3 participants