-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add Nette integration #504
Conversation
02b81b8
to
4a9d970
Compare
4a9d970
to
b327b54
Compare
Wow! Thanks for this awesome contribution @kozaktomas! |
b327b54
to
de0f3b9
Compare
c9a5e3d
to
888a7e1
Compare
888a7e1
to
93ff013
Compare
f32634e
to
e7f3415
Compare
Hi @pawelchcki I rebased my branch few times. It would be really nice to have it in the next version of this package. 🙏 |
Thank you so much for this awesome PR @kozaktomas! My apologies for the delay for a code review. I'll make sure to get this reviewed this week. :) |
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.
Fantastic work @kozaktomas! No changes required. I'm approving this now and will discuss with the team if we can get it merged for the next release. If not, we'll most likely merge it for the following release.
Thanks again for such a clean and feature-complete PR! ❤️
@kozaktomas We've got this slated for not the next release, but the release after that. That will give us some time to add the new Sandbox API to your awesome integration and then we can get it released. :) |
e7f3415
to
e48c8b6
Compare
Hi @SammyK. I did deeper testing (with agent) and I found a bug. I've added one additional commit. I also did rebase with master. Sorry for inconvenience. :) |
Hi there! I am trying DataDog APM with app based on Nette Framework right now and found this PR, why is it closed and not merged? It seems to me like it has already been almost done. |
This was completely my fault for missing this for release so long ago. I'm sorry about that @kozaktomas. @roman-vohnik This PR uses the legacy API for the integration which no longer ships with the tracer, but we should still be able to use the fantastic work @kozaktomas has already done and update it to use the sandbox API. Unfortunately we're not able to provide an ETA on when we will be able to refactor this at the moment, but I have added this to our internal project tracking and it is on our radar. |
This PR for the Nette framework integration (#489) has been revived and merged via #1220 and will be included in the next release. Thank you again @kozaktomas for the original work on this and thank you @Anilm3 for reviving this PR. My apologies again for letting this PR slip through the cracks. |
Supported Nette framework versions:
Both versions are tested.
Integration is loaded after
Nette\Configurator::__construct
. It's one of the first object created in Nette.Spans:
Closes #489
Readiness checklist