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

updated version of the C++ Getting Started with a Roll The Dice #3423

Merged
merged 68 commits into from
Feb 9, 2024

Conversation

Akhaled19
Copy link
Contributor

@Akhaled19 Akhaled19 commented Oct 21, 2023

This is to solve issue #3345.
Unfortunately, it's not quite ready yet as I'm currently encountering some challenges while configuring OpenTelemetry with my oatpp Application. I would greatly appreciate any tips or assistance in this matter.


Preview:

@Akhaled19 Akhaled19 requested review from a team October 21, 2023 20:40
Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

thanks for taking the time and effort to write this down @Akhaled19! My first review is just from taking a look at the text, I will try out what you have created so far and will provide additional feedback

@Akhaled19
Copy link
Contributor Author

Hello @svrnm! I made the switch from conan to source code and fixed up the instructions. I made a rookie mistake of not unstaging some of those files and keeping only content/en/docs/instrumentation/cpp/getting-started-new.md before my commit. I apologize for the messy pull request.

@svrnm
Copy link
Member

svrnm commented Nov 6, 2023

Hello @svrnm! I made the switch from conan to source code and fixed up the instructions.

thanks, let me take another look!

I made a rookie mistake of not unstaging some of those files and keeping only content/en/docs/instrumentation/cpp/getting-started-new.md before my commit. I apologize for the messy pull request.

No worries, we will squash merge the PR at the end, so no mess to see :-)

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

Hey @Akhaled19, first of all: it works! 🎉 -- I could follow those instructions and got "myapp" export a span to the console, that's really big progress.

I struggled with the locations of the dependencies a little bit, we need to tune that in a way that it is easy for end-users to consume. Speaking as a non-C++-expert I am wondering if instead of running a make install just build them and load the include files and libraries from the build folders, that way you can provide a path to the end users, because they should have a folder with the following structure:

oatpp
opentelemetry-cpp
roll-dice

And in the CMakeLists for roll-dice they should be able to use ../oatpp/<path-to-lib|include> and ../opentelemetry-cpp/<path-to-lib|include>

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

A few initial comments.

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

It works 🎉 ... some minor comments, but overall this is looking really good!

@svrnm
Copy link
Member

svrnm commented Jan 31, 2024

@open-telemetry/cpp-approvers please take a look

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions.

@svrnm
Copy link
Member

svrnm commented Feb 8, 2024

@lalitb @marcalff thanks for taking a look! @Akhaled19 please take a look at the open conversations & the conflict, let me know if you need any assistance to fix them.

@svrnm svrnm merged commit f1b3d18 into open-telemetry:main Feb 9, 2024
14 checks passed
@svrnm
Copy link
Member

svrnm commented Feb 9, 2024

thank you soo much for your work on this issue @Akhaled19 !!!

https://opentelemetry.io/docs/languages/cpp/getting-started/

@Akhaled19
Copy link
Contributor Author

Thank you for making my first issue a pleasant experience @svrnm!!

@svrnm
Copy link
Member

svrnm commented Feb 12, 2024

Thank you for making my first issue a pleasant experience @svrnm!!

Happy to hear that! This PR was really a big challenge due to the complexity of the task, so I am glad you had a great experience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig:cpp sig-approval-missing Co-owning SIG didn't provide an approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants