Merged
Conversation
brrygrdn
commented
May 11, 2022
3192e46 to
d5afce6
Compare
jakecoffman
approved these changes
May 11, 2022
Member
jakecoffman
left a comment
There was a problem hiding this comment.
I like that this makes dry-run output closer to what we see in the job logs. Should we maybe have it on by default?
Contributor
Author
|
@jakecoffman Yeah, that's a good point, I don't see the harm in turning it on by default as I think since network calls are our biggest expense keeping them visible is no bad thing |
landongrindheim
approved these changes
May 12, 2022
Contributor
landongrindheim
left a comment
There was a problem hiding this comment.
This is awesome 🙌
d5afce6 to
31fa596
Compare
Merged
This file contains hidden or 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
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.
We are currently investigating some slow running jobs and one thing that is noticeable is that some ecosystem implementations tend to make excessive network calls within core's code.
This change does two things:
ActiveSupport::Notificationas a generic instrumentor forExcon, per the docs--network-traceargument to the dry-run script that optionally prints out every request made during the run along with a total count at the end of the jobExample:
Potential follow-ups
The use of ActiveSupport notification may pave the way for attaching a metric service to core in future.