-
Notifications
You must be signed in to change notification settings - Fork 440
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
Allow Rails to provide timing data in browser dev-tools via Server-Timing header #1771
Merged
joelhawksley
merged 20 commits into
ViewComponent:main
from
tgaff:rails-server-timings-support
Jul 7, 2023
Merged
Changes from 12 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
ee505b4
change ActiveSupport::Notification event name so it is included in Ra…
tgaff fa874f5
Changelog update for instrumentation
tgaff 62e02a9
update docs for instrumentation change
tgaff 1468c16
add trailing whitespace per PR linter
tgaff f2a8a91
Instrumentation key with '!' deprecated and usage can be configured
tgaff 4d9a5c2
update CHANGELOG to mention new config for instrumentation and deprec…
tgaff 107b610
update documentation for instrumentation
tgaff 875f6bd
send deprecation notice once, rather than on each render
tgaff 5ea279f
remove double-spaces
tgaff e402c80
Merge branch 'main' into rails-server-timings-support
joelhawksley 2158ad3
Apply suggestions from code review
joelhawksley 1c95b54
Merge branch 'main' into rails-server-timings-support
joelhawksley 7bce027
Update docs/guide/instrumentation.md
joelhawksley 7b28a16
Merge branch 'main' into rails-server-timings-support
joelhawksley 9fe8dd3
rename instrumentation config key to match other renames
tgaff 1ad2f0d
nocov initializer code: re-executing it in tests causes order-depende…
tgaff dd05dc8
lint
tgaff e4b5993
docs: Add browser dev tools image to aid explanation
tgaff e570a63
docs: code back-ticks to make Vale happier
tgaff 22afad0
md linting
tgaff File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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.
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.
Not important but: specifying Chrome here is perhaps a little "chrome-centric". The feature works nearly the same in FF, Safari, Edge and Opera. The screenshot in the PR is from FF 113 IIRC.
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.
Is there anyway you can add Firefox and safari instructions
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.
They are nearly identical aside from coloration, so I don't think it's necessary.
Biggest caveats with Server-Timings in general:
From top-left going clockwise: Chrome, Firefox, Safari. You can see its pretty much the same.
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.
How about we just say "browser" here? Perhaps we should add this screenshot to the docs?
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.
I just added a smaller variant of the screenshot. When running the docs locally that path seems to work and display properly, but there may be some risk of a dead img src when deployed.