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.

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

Jump to bottom

Feat: Add Redis watcher #293

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Feat: Add Redis watcher #293

merged 8 commits into from
Sep 23, 2024

Conversation

MilesChou
Copy link
Contributor

Call facade Redis::enableEvents() before using this watcher.

@MilesChou MilesChou requested a review from a team September 6, 2024 04:04
@codecov Codecov
Copy link

codecov bot commented Sep 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.68%. Comparing base (9b97522) to head (5d3c2ec).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #293      +/-   ##
============================================
- Coverage     84.80%   82.68%   -2.12%     
- Complexity      947      949       +2     
============================================
  Files            80       89       +9     
  Lines          3798     3807       +9     
============================================
- Hits           3221     3148      -73     
- Misses          577      659      +82
Flag Coverage Δ
Aws 85.75% <ø> (ø)
Context/Swoole 0.00% <ø> (ø)
Instrumentation/CodeIgniter 73.94% <ø> (ø)
Instrumentation/ExtAmqp 89.58% <ø> (ø)
Instrumentation/Guzzle 69.73% <ø> (ø)
Instrumentation/HttpAsyncClient 81.33% <ø> (ø)
Instrumentation/IO 70.90% <ø> (ø)
Instrumentation/MongoDB 77.33% <ø> (ø)
Instrumentation/OpenAIPHP 86.82% <ø> (ø)
Instrumentation/PDO 89.56% <ø> (ø)
Instrumentation/Psr14 78.12% <ø> (ø)
Instrumentation/Psr15 93.50% <ø> (ø)
Instrumentation/Psr16 ?
Instrumentation/Psr18 82.08% <ø> (ø)
Instrumentation/Psr3 ?
Instrumentation/Psr6 ?
Instrumentation/Slim 86.95% <ø> (ø)
Instrumentation/Symfony 89.03% <ø> (ø)
Instrumentation/Yii 77.77% <ø> (ø)
Logs/Monolog 100.00% <ø> (ø)
Propagation/ServerTiming 100.00% <ø> (ø)
Propagation/TraceResponse 100.00% <ø> (ø)
ResourceDetectors/Container 93.02% <ø> (ø)
Sampler/RuleBased 32.14% <ø> (?)
Shims/OpenTracing 92.99% <ø> (ø)
Symfony 88.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 17 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b97522...5d3c2ec. Read the comment docs.

@MilesChou
Copy link
Contributor Author

@brettmc How can I improve this PR.

@brettmc
Copy link
Collaborator

brettmc commented Sep 16, 2024

How can I improve this PR.

I think we looked at this in last week's SIG meeting, and this one was good to merge, @ChrisLightfootWild ?

@ChrisLightfootWild
Copy link
Contributor

How can I improve this PR.

I think we looked at this in last week's SIG meeting, and this one was good to merge, @ChrisLightfootWild ?

Apologies for my very delayed response @MilesChou - I did promise to take a look at both of your PRs this week and I have unfortunately only just gotten around to it. Sorry! ❤️

MilesChou and others added 3 commits September 18, 2024 09:59
@MilesChou @ChrisLightfootWild
Update src/Instrumentation/Laravel/src/Watchers/RedisCommand/RedisCom…
5227a8c 
…mandWatcher.php

Co-authored-by: Chris Lightfoot-Wild <github-clw@wild.me.uk>
@MilesChou
Return null when get db index
014b591 
Don't cause application problem.
@MilesChou
Renaming
86fe930
ChrisLightfootWild
ChrisLightfootWild reviewed Sep 18, 2024
View reviewed changes
src/Instrumentation/Laravel/src/Watchers/RedisCommand/RedisCommandWatcher.php Outdated
TraceAttributes::DB_SYSTEM => TraceAttributeValues::DB_SYSTEM_REDIS,
TraceAttributes::DB_NAME => $this->fetchDbIndex($event->connection),
TraceAttributes::DB_OPERATION => $operationName,
TraceAttributes::DB_QUERY_TEXT => Serializer::serializeCommand($event->command, $event->parameters),
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be unavailable in semconv 1.24, which is the target for a 1.0 release (and the last supported version for php 8.0).

Not sure if there is an alternative which might be better suited here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use DB_STATEMENT for compatibility with semconv 1.24.

Ref: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.19.0/specification/trace/semantic_conventions/database.md#redis

src/Instrumentation/Laravel/src/Watchers/RedisCommand/RedisCommandWatcher.php Outdated Show resolved Hide resolved
src/Instrumentation/Laravel/src/Watchers/RedisCommand/RedisCommandWatcher.php Outdated Show resolved Hide resolved
@MilesChou MilesChou requested a review from a team as a code owner September 19, 2024 02:17
@brettmc brettmc merged commit 80afd21 into open-telemetry:main Sep 23, 2024
95 of 120 checks passed
@MilesChou MilesChou deleted the add-laravel-redis-connection branch September 23, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisLightfootWild ChrisLightfootWild ChrisLightfootWild approved these changes

@brettmc brettmc brettmc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MilesChou @brettmc @ChrisLightfootWild