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

Pass collectDelay arg to Clinic Doctor/Flame #208

Merged
merged 11 commits into from
Mar 26, 2020
Merged

Conversation

DylanC
Copy link
Contributor

@DylanC DylanC commented Feb 4, 2020

Related to this PR:

Doctor
clinicjs/node-clinic-doctor#295

Flame
clinicjs/node-clinic-flame#107
0x
davidmarkclements/0x#222

BubbleProf - Will be separate from this change
clinicjs/node-clinic-bubbleprof#346

The "--collect-delay" arg will be passed in with the arguments and allow the collection of data to be delayed.

@DylanC
Copy link
Contributor Author

DylanC commented Feb 5, 2020

Once this is working for all three I will need to update the parameter from -t to --on-timeout.

@DylanC DylanC changed the title Pass timeoutDelay arg to Clinic Doctor Pass timeoutDelay arg to Clinic Doctor/Flame/Bubbleprof Feb 7, 2020
@DylanC
Copy link
Contributor Author

DylanC commented Feb 7, 2020

@jasnell
This feature may not work for BubbleProf. Discovered this is difficult/maybe not possible when discussing with @goto-bus-stop. The timeout creates an async entry that BubbleProf can't seem to track. The whole design of BubbleProf relies on a valid root sourceNode that the grouped child Nodes can trace back to.

Will need to think about a solution or only offer the time delay for Doctor and Flame.

@jasnell
Copy link
Contributor

jasnell commented Feb 7, 2020

Yeah it's one of the key limitations of bubbleprof I'd say. That that one who may need to collect everything but figure out a way to reasonably filter during the analysis based on timestamp.

@DylanC
Copy link
Contributor Author

DylanC commented Feb 7, 2020

@jasnell - Alright, I'll look into this some more and see what could be possible.

@jasnell
Copy link
Contributor

jasnell commented Feb 7, 2020

For now, I'd say limit the timeout feature to doctor and flame an set the bubbleprof support as a separate task.

@DylanC DylanC requested a review from goto-bus-stop February 7, 2020 15:47
@DylanC
Copy link
Contributor Author

DylanC commented Feb 11, 2020

@goto-bus-stop - Can I get you to review this and related PR's at some stage today?

@DylanC DylanC changed the title Pass timeoutDelay arg to Clinic Doctor/Flame/Bubbleprof Pass collectDelay arg to Clinic Doctor/Flame Feb 11, 2020
@DylanC
Copy link
Contributor Author

DylanC commented Feb 13, 2020

@jasnell - Updated with 0x draft PR.

@DylanC DylanC merged commit e6db361 into master Mar 26, 2020
@goto-bus-stop
Copy link
Contributor

We should really only merge PRs like this once the other packages have been released, so we don't depend on temporary branches in the package.json

@DylanC
Copy link
Contributor Author

DylanC commented Mar 30, 2020

Ah yes! Makes sense!

goto-bus-stop added a commit that referenced this pull request May 29, 2020
@DylanC DylanC deleted the collect_timeout_feature branch May 29, 2020 14:54
@ErisDS
Copy link

ErisDS commented Aug 17, 2021

Can the revert be reverted? This would be super useful...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants