-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat(crwa): OpenTelemetry in CRWA with a new TUI #8043
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtoar
approved these changes
Apr 19, 2023
16 replays were recorded for 0b4e4d7. 16 PassedrequireAuth graphql checks
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DRAFT/WIP: I still have some things I want to change about this. The telemetry backend isn't permanent yet as such it is likely best to test with
--telemetry false
. Overall, I'm not certain we'll collectively think it's a good idea to go in this direction with atui
package.Problem
We want to switch out to use OpenTelemetry in create-redwood-app and in doing so reduce create-redwood-app's dependency size.
Changes
@redwoodjs/tui
.Note on point 2
We currently use
@opentelemetry/exporter-trace-otlp-http
as the OpenTelemetry exporter but it's surprisingly heavy at around 5MB! We might want to try find an exporter with a smaller footprint.@opentelemetry/exporter-zipkin
is around half the size but I'll need to do some research on this.Note on point 3
The switch to OTel was started in #7455 but I was unhappy working around Listr2 when trying to instrument the code. Listr2 is also quite heavy at around 5MB. I went ahead and stripped it out and replaced it with a basic homebrewed TUI package -
@redwoodjs/tui
.My thinking was we can reduce the size of the dependencies and have a little more control over the look and feel of the terminal experience. This may be over-engineering things a little but the tui package could also serve as a good way to keep styling consistent between crwa, cli and any cli plugins we enable in the future. I didn't think we needed a full task list rendering library like
tasktree
for create-redwood-app but we'll likely need that for the CLI - perhaps we can implement something similar or use such a lib. Happy to discuss concerns or objections around going in this direction!