-
Notifications
You must be signed in to change notification settings - Fork 999
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
OpenTelemetry PHP Auto-Instrumentation blog post #2506
OpenTelemetry PHP Auto-Instrumentation blog post #2506
Conversation
053ab2b
to
dcc1d52
Compare
dcc1d52
to
c3d0dfe
Compare
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.
This looks great, I've suggested some small changes for readability etc :)
eeb3d67
to
990c164
Compare
@brettmc Thank you for the review. I applied your suggestions |
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.
Great read, a few suggestions.
@svrnm Thank you for suggestions. Applied changes |
bdc341e
to
b529e1f
Compare
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.
See inline comments. The docsy change must be resolved (dropped) before this lands.
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
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.
Haven't read through in detail, will go with the feedback of @svrnm & others for that, but aside from a few code-block language issues (noted inline), LGTM.
02696a8
to
912b853
Compare
Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
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.
Thank you! This is really exciting. I love seeing this project come to life and it will be fantastic to announce it to the world.
Some minor suggested changes from my part. Basically, as the project stabilizes I'd like to find a way to bring it into the docs. Pulling content from the post here will save us time an energy, so if we change the post a little bit to be slightly more docs-like, we'll probably thank our past selves in the future.
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.
Some small touchups (if they're valid still, I see changes went through). Awesome blog post! I'll be reading into all of this more 😁
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
cbb2697
to
1dc7bdc
Compare
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, great updates.
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.
LGTM, just one typo found in my last proof-read.
Co-authored-by: Brett McBride <brett.a.mcbride@gmail.com>
@open-telemetry/docs-maintainers Can we merge it? |
I still see a few open suggestions, can you quickly double check them and close them as resolved |
@svrnm Now should be ok. I resolved open suggestions and updated one small thing. |
No description provided.