-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 a React Native example app to the demo #1781
Add a React Native example app to the demo #1781
Conversation
@jpmunz this looks great! We had some similar work presented at a previous SIG meeting some time ago by @breedx-splk 's team. Are you able to join a OTel Demo SIG meeting to present this to everyone? Also, adding a README on how to run the service would help a lot. Regarding the doc page, yes, that would be under demo/services, but it may take a while to review this PR as it is a 25K lines of code addition. |
I don't believe that they were...but this is super interesting. React native on mobile is asked about quite a lot -- so having this available in/through the android app would also be amazing and helpful. It would allow us to demonstrate the client-side RUM interop (currently nonexistent!) and compare/contrast features. Logged this issue to track: open-telemetry/opentelemetry-android#696 |
You can move forward with committing the generated proto files. We recently launched an effort to do the same for all other services in the Demo. |
@julianocosta89 I was not involved in that previous meeting but I'm happy to join the next one (Nov 27th?) and present there!
I had included https://github.com/open-telemetry/opentelemetry-demo/blob/3404e12a242936bab02ad3a4d5e093fdb59839d1/src/reactnativeapp/README.md, did that include what you had in mind or was there something else I could add? |
@breedx-splk that's great would be cool if the UI here could be re-used to demo the Android SDK. Another possible avenue that I'll put out there could be using RN Native Modules to wrap the Android (and potentially Swift) OTEL SDKs in a thin JS package. That might be less flexible but could be appealing to devs who didn't want to deal with the setup on the native side and just wanted to add a JS package that handled it for them |
Yes! Nov 27th, but I'll try to put some time into reviewing the PR till there.
This is perfect, I just missed it 😅 |
Yeah I think we will need both native with RN inside and potentially this long term. There will always be devs that just write pure native Android, and we need to make sure we support them first. |
@jpmunz could you add some instructions on how to get this PR up and running to someone that has 0 knowledge on reactive native and mobile development? Following the README didn't work for me, it seems that I'm missing 1 million tools to be able to run it. Also, the ios version, I'm getting this error:
Not sure if that is something related to permissions. As I said, I have 0 knowledge on mobile development 😢 |
hey @julianocosta89, in the README I had included a link to https://reactnative.dev/docs/set-up-your-environment for first time setup which I was hoping would cover things in a bit more depth, were there particular bits of information you think would be better to mention explicitly in the README? You could run on a physical Android or iOS device which might save some setup steps over launching an emulator though that would still require building the app on one of the platforms For that particular error you could try running |
@julianocosta89 hmm so in that case it seems to have built the app correctly but can't load the JS bundle, in debug builds the bundle is grabbed from a local server instead of being packaged with the app. This local server should have been started automatically as part of Hitting "Reload" on that error screen might also sometimes do the trick if the server is up but it wasn't able to connect with it right away |
@jpmunz running |
ah nice, glad it worked! ya running it through Xcode can be helpful if there's a problem as it'll give more readable output than the CLI but then need to run the dev server separately (or do a release build). I also tend to run |
Do we need the After resetting my environment and cloning this PR's branch, when I run this for iOS, those 2 files get updated. Should these 2 files be in |
I've tested with a clean workspace and I actually need to first run the |
I'd go vote for |
Updated to react-native-app: 7b29f63 Docs repo change: open-telemetry/opentelemetry.io@61d5208 |
ah that's right, updated instructions here: 76e5cbe |
hey @puckpuck normally both these files are committed to version control, could you paste the diff you get after running these commands? I could take a look and try and see where the differences are coming from, there may be some tooling discrepancy which we may also have to lockdown |
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.
Add 3 comments, but to me it is ready to go!
src/react-native-app/ios/reactnativeapp/reactnativeapp.entitlements
Outdated
Show resolved
Hide resolved
I got the following:
|
ah ya we're not pinning the cocoapods version, I made an update here that hopefully locks that down: 1e7f121 I added a note to the README unfortunately it adds a few more steps to the setup on iOS so I worded it as something optional, let me know what you think |
@puckpuck unfortunately I wasn't able to reproduce this, googling around a bit it seems like it could be an issue with the You had mentioned above seeing some local changes to |
Yes, my issue was |
I'm good with this. I want to make some changes to the Readme. We could have better structure and move some of the content into different sections, but I prefer we merge this first. Thank you again, @jpmunz, for this fantastic PR! 🤩 |
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.
@jpmunz would you be able to solve the merge conflicts?
This should be good to go!
Really appreciate your patience in guiding me through here, and all the back and forth 🙏🏽
@julianocosta89 @puckpuck thank you both for your encouragement and taking the time to dive into and review this very large PR! |
@julianocosta89 merge conflicts should be resolved now looks like
|
@jpmunz it looks like there still conflict 🥲 If you could resolve it, I can get it merged right away |
Don't worry about the broken link. It is not related to your PR |
@julianocosta89 np, updated! |
Changes
Hello! I thought it would be valuable to include a React Native example app in this demo to show how mobile apps could be instrumented using the existing JS OTEL packages. I also thought this could serve as a bit of a TODO list showing some of the rough edges / missing functionality that current React Native developers would face when trying to instrument and hopefully keep updating this as those gaps are filled
A note on the size of the diff: unfortunately there's a ton of boilerplate files that are required for just spinning up a blank new app that make this harder to review. I have these on a separate commit but don't have permission to push that up as a branch, if someone could do that this PR could then be pointed to it and only show the files that are relevant to the demo similar to what you'd see on cde1345. Otherwise using the file filters to hide things like the .png, .xml, etc. will help narrow down to the relevant changes
Some notes on the overall approach here curious to hear thoughts or disagreement:
src/frontend
since that is also using React. However there were enough tweaks needed to make it work with React Native that it didn't seem straightforward to try and have these two apps share common files and also I think useful for demo purposes to have each be self-containedI haven't yet but I'm tempted to commitcommitted nowprotos/demo.ts
andpb/demo.proto
just for this component, it's a bit weird since they are generated files but since this app isn't run inside Docker users will have to generate those files themselves which adds a bit of friction for running the demoUsing the demo:
Merge Requirements
For new features contributions, please make sure you have completed the following
essential items:
CHANGELOG.md
updated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.