Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
👷 Laravel instrumentation 1.1.x #269
base: main
Are you sure you want to change the base?
👷 Laravel instrumentation 1.1.x #269
Changes from 9 commits
4c7f2c9
e2e039a
65e413e
1deb2b4
98af07f
1ba04c2
f3428c5
bb09763
c1ed471
3d9beb3
9788fdd
60ba83a
d5cdeca
e494ac2
ea2e4bb
4231acd
ea05ad4
d67da56
190f628
7412881
d2887bc
cf6deaf
c53a6db
3eec435
45a6932
ee8cebe
a6a677e
6a8d0ef
60fc781
0d7dcf4
16dc809
fc1df3f
c9a83fe
44440df
a3cf950
504796e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed based on similar issue raised for PDO instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I actually be passing the
*ProviderInterfaces
around?My current understanding would be that I'd have to duplicate the instantiation or provide a Factory to give me back the implementations I want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think you should. I also think you should just accept the
InstrumentationContext
here, and retrieve the providers you need for this particular hook (less rework if we add new providers in future). Also, I think the Tracer/Logger/etc for this hook should have a unique name that includes the component being instrumented, egio.opentelemetry.contrib.php.laravel.kernel
- the reason for this is that we can use scope configuration to turn off parts of auto-instrumentation based on that name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @brettmc, all of that sounds sensible. I will look to make the switch.
I will have to have a tinker with the scope configuration here and see how that goes!