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

Improves "emulators:start" logs #2219

Merged
merged 17 commits into from
May 14, 2020
Merged

Improves "emulators:start" logs #2219

merged 17 commits into from
May 14, 2020

Conversation

abeisgoat
Copy link
Contributor

@abeisgoat abeisgoat commented May 7, 2020

Updates emulators:start to show...

✔  emulators: All emulators started, it is now safe to connect.
┌───────────┬────────────────┬─────────────────────────────────┐
│ Emulator  │ Host:Port      │ View in Browser                 │
├───────────┼────────────────┼─────────────────────────────────┤
│ Functions │ localhost:5001 │ http://localhost:4001/functions │
├───────────┼────────────────┼─────────────────────────────────┤
│ Firestore │ localhost:8080 │ http://localhost:4001/firestore │
├───────────┼────────────────┼─────────────────────────────────┤
│ Database  │ localhost:9000 │ http://localhost:4001/database  │
├───────────┼────────────────┼─────────────────────────────────┤
│ Hosting   │ localhost:5000 │                                 │
├───────────┼────────────────┼─────────────────────────────────┤
│ Pubsub    │ localhost:8085 │                                 │
└───────────┴────────────────┴─────────────────────────────────┘

You can also view status and logs of the emulators by pointing your browser to http://localhost:4001/.

Issues? Report them at https://github.com/firebase/firebase-tools/issues and attach log files named *-debug.log in current directory.

@abeisgoat abeisgoat requested a review from yuchenshi May 7, 2020 03:58
@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label May 7, 2020
Copy link
Member

@yuchenshi yuchenshi left a comment

Choose a reason for hiding this comment

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

Could you please also post a screenshot and a full log of all messages printed during start? I think we can clean up more cruft and we can talk more about it on chats.

CHANGELOG.md Outdated Show resolved Hide resolved
src/commands/emulators-start.ts Outdated Show resolved Hide resolved
@abeisgoat
Copy link
Contributor Author

Screen Shot 2020-05-07 at 1 21 00 PM

Here's current logs, we could potentially drop the messages for with port and debug log file for each emulator and move the log file name into the table. That plus removing all the GUI setup logic which prints out will help

@samtstern
Copy link
Contributor

@abeisgoat how does this kind of thing look in the logging emulator? And yeah let's definitely remove the {emulator} started at {address} messages above the table, those are dupes now.

@abeisgoat
Copy link
Contributor Author

Another pass based on feedback from bash.

@samtstern it breaks terribly, open to feedback about how we should handle it.

Screenshot from 2020-05-10 23-53-45

@samtstern
Copy link
Contributor

@abeisgoat logs LGTM but I am worried about how it looks in the logs viewer. Can we just omit it somehow? Maybe add a new utils.logFancy or something like that?

I assume that for things like the emulator download progress bar or ora spinners we don't go through our normal logger. Tables should probably be the same.

@abeisgoat
Copy link
Contributor Author

@samtstern we could def just write it to stdout which would pass the logger. prolly the best call. I'll do that fix and wait for @yuchenshi lgtm

@abeisgoat
Copy link
Contributor Author

Screenshot from 2020-05-12 14-09-58

One more pass, the only concern is that the table is 86 characters wide and breaks anytime the console is less than that. Do we can? idk

@abeisgoat
Copy link
Contributor Author

also got it looking correct in the logs viewer

Screenshot from 2020-05-12 14-45-47

@yuchenshi
Copy link
Member

One more pass, the only concern is that the table is 86 characters wide and breaks anytime the console is less than that. Do we can? idk

I have but one suggestion -- remove log files from the table and throw them into the Issues? line since they are only needed for troubleshooting and bug reports. This way we can make the table width safe enough.

As a side topic, we should consider surfacing those logs in the GUI logs viewer once the logs transportation changes are in (maybe with log level "DEBUG" or "VERBOSE") and then there will be even one less reason to inspect the files manually. Especially the rules debug and rejection logs in Firestore emulator.

src/commands/emulators-start.ts Outdated Show resolved Hide resolved
@abeisgoat
Copy link
Contributor Author

note to self: move hub port to "other reserved ports" line

@abeisgoat
Copy link
Contributor Author

Screenshot from 2020-05-13 17-17-44

another pass

@abeisgoat
Copy link
Contributor Author

aight, one more time, thoughts / complaints @samtstern @yuchenshi ?

@samtstern
Copy link
Contributor

LGTM

@yuchenshi
Copy link
Member

One last thing: please drop localhost:4000/functions -- that's not a real thing. And feel free to merge after that.

@yuchenshi
Copy link
Member

Screen Shot 2020-05-13 at 10 11 54 PM

FYI The first line seems missing on my side in the logs viewer. Not blocking, but I'm super curious why.

@abeisgoat
Copy link
Contributor Author

@yuchenshi That's fixed over here. We were doing a super hack split up logs which were (almost) always in the format of ICON EMULATOR: MESSAGE to cut off the ICON part and move it into the button in the adjacent column.

However, now we ship the log in chunks so there's no string parsing involved and cases where the string doesn't fit that format no longer break.

@yuchenshi
Copy link
Member

Okay, I think this PR is good to go once we merge that PR and cut a release this afternoon.

@abeisgoat abeisgoat merged commit 66228c0 into master May 14, 2020
@abeisgoat abeisgoat deleted the ah/emulator-start branch May 14, 2020 21:33
@yuchenshi
Copy link
Member

(FYI this is safe for releasing -- the link works in the GUI build I just cut.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants