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

[Demo] Add info about React Native app #5704

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jpmunz
Copy link

@jpmunz jpmunz commented Nov 28, 2024

This updates the demo documentation to include the React Native example app that was added here: open-telemetry/opentelemetry-demo#1781

Preview: https://deploy-preview-5704--opentelemetry.netlify.app/docs/demo/services/react-native-app/

@opentelemetrybot opentelemetrybot requested a review from a team November 28, 2024 15:30
content/en/docs/demo/_index.md Outdated Show resolved Hide resolved
content/en/docs/demo/services/reactnativeapp.md Outdated Show resolved Hide resolved
content/en/docs/demo/services/reactnativeapp.md Outdated Show resolved Hide resolved
@opentelemetrybot opentelemetrybot requested a review from a team December 2, 2024 14:11
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, looks good overall, a few comments

content/en/docs/demo/services/reactnativeapp.md Outdated Show resolved Hide resolved
content/en/docs/demo/services/reactnativeapp.md Outdated Show resolved Hide resolved
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.

As a general comment, I don't think that the app should be documented as a part of the demo/services. Even the entry in the index page reads, "React Native mobile application that provides a UI on top of the shopping services."

How does it relate to the Collector?

Maybe we need a new Demo top-level entry?

@chalin chalin changed the title Demo docs update for the react native example app [Demo] Add info about React Native app Dec 17, 2024
@julianocosta89
Copy link
Member

As a general comment, I don't think that the app should be documented as a part of the demo/services. Even the entry in the index page reads, "React Native mobile application that provides a UI on top of the shopping services."

Maybe we need a new Demo top-level entry?

I actually disagree @chalin.
The app is just another way to interact with the demo.
The user can navigate to the frontend and buy astronomy stuff, or they can open the app and do the same.
The app should be on the same level as frontend-proxy, which is the service that exposes the frontend.

How does it relate to the Collector?

As well as the other services, the reactive-native-app is instrumented with OTel and sends traces to the Collector.

@puckpuck
Copy link
Contributor

I agree with @julianocosta89 here. The react-native-app could be seen similar to the frontend service. It the mobile app does route all its telemetry data through the frontend-proxy into a collector, and does similar API calls as the web tier does.

@jpmunz jpmunz marked this pull request as ready for review December 19, 2024 21:27
@jpmunz jpmunz requested a review from a team as a code owner December 19, 2024 21:27
@jpmunz
Copy link
Author

jpmunz commented Dec 19, 2024

Marking this as Ready to Review now that the associated PR in the demo repo got merged: open-telemetry/opentelemetry-demo#1781 (comment)

Will updated and take a look at the check failures

@jpmunz
Copy link
Author

jpmunz commented Dec 19, 2024

I'm not quite sure how to address the failure on the "Build and Check Links" check, it appears to fail on running npm run _diff:fail which exits successfully for me locally

@jpmunz jpmunz requested review from svrnm and chalin December 19, 2024 22:32
@tiffany76
Copy link
Contributor

/fix:htmltest-config

@opentelemetrybot
Copy link
Collaborator

You triggered fix:htmltest-config action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/12422284976

@opentelemetrybot
Copy link
Collaborator

fix:htmltest-config was successful.

IMPORTANT: (RE-)RUN /fix:all to ensure that there are no remaining check issues.

@tiffany76
Copy link
Contributor

/fix:all

@opentelemetrybot
Copy link
Collaborator

You triggered fix:all action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/12422305906

@opentelemetrybot
Copy link
Collaborator

fix:all failed or was cancelled. For details, see https://github.com/open-telemetry/opentelemetry.io/actions/runs/12422305906.

@tiffany76
Copy link
Contributor

I'm not quite sure how to address the failure on the "Build and Check Links" check, it appears to fail on running npm run _diff:fail which exits successfully for me locally

I think the issue is fixed, but the CI checks need to be triggered again. Maybe try a rebase?

@svrnm
Copy link
Member

svrnm commented Dec 20, 2024

@jpmunz @tiffany76 let's ignore the build issues for now, we can fix them once the PR is approved by @open-telemetry/demo-approvers and @open-telemetry/docs-approvers and before we merge it

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

LGTM 📱📱📱

Copy link
Contributor

@tiffany76 tiffany76 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 a few small copy edits.

## Instrumentation

The application uses the OpenTelemetry packages to instrument the application at
the JS layer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the JS layer
the JS layer.

Comment on lines +20 to +24
The JS OTel packages are supported for node and web environments. While they
work for React Native as well they are not currently explicitly supported for
that environment and may break compatibility with minor version updates or
require workarounds. Building more support for React Native is an area of active
development.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The JS OTel packages are supported for node and web environments. While they
work for React Native as well they are not currently explicitly supported for
that environment and may break compatibility with minor version updates or
require workarounds. Building more support for React Native is an area of active
development.
The JS OTel packages are supported for node and web environments. While they
work for React Native as well, they are not explicitly supported for
that environment, where they might break compatibility with minor version updates or
require workarounds. Building JS Otel package support for React Native is an area of active
development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

7 participants