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

Program with profiler enabled may not exit for up to a hour unless CTRL-C'ed #28

Open
nolanmar511 opened this issue Oct 26, 2017 · 8 comments
Labels
api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. type: cleanup An internal cleanup or hygiene concern.

Comments

@nolanmar511
Copy link
Contributor

The request in createProfile() can hang for up to an hour. So, when a user's code finishes running, it could be up to an hour before profiler exits (and node.js can stop running).

@nolanmar511 nolanmar511 self-assigned this Oct 26, 2017
@nolanmar511
Copy link
Contributor Author

Copied from aalexand's comment on PR #16:

There was a question on how CreateProfile call (which is hanging) can be cancelled, as I think unless it is cancelled the Node.js program being profiled won't exit? Looking at the code of google-cloud-node/common, some thoughts:

  • makeAuthenticatedRequestFactory in util.js does seem to support cancellation via abort() on the active request - see here.
  • Unfortunately in the service.js file the return value seems to be dropped as there isn't a return statement. I don't know why.
  • It is also dropped down the chain here and here.
  • Also, all methods on a service object are wrapped with util.promisifyAll, here. The wrapper only returns a promise if no callbacks are passed though - if there are callbacks passed, it returns the original value.

So, I would naively expect that supporting cancellation of CreateProfile call is as easy as:

  • Change the code of service.js and service-object.js to return the object with the abort() call for non-streaming requests.
  • Change the profiler agent call to pass callbacks for the CreateProfile's request() call to promisify the call manually to gain the control over abort() call and call that on program exit.
  • Not sure how to intercept into the program exit. There is process.on('exit', ...) but not sure if there is a chicken-and-egg issue with it.

@aalexand aalexand changed the title Stopping createProfile() when event loop is empty otherwise. Program with profiler enabled won't exit unless CTRL-C'ed Nov 19, 2017
@JustinBeckwith JustinBeckwith added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. 🚨 This issue needs some love. labels Sep 13, 2018
@nolanmar511 nolanmar511 changed the title Program with profiler enabled won't exit unless CTRL-C'ed Program with profiler enabled may not exit for up to a hour unless CTRL-C'ed Feb 12, 2019
@nolanmar511 nolanmar511 added priority: p3 Desirable enhancement or fix. May not be included in next release. priority: p2 Moderately-important priority. Fix may not be included in next release. type: cleanup An internal cleanup or hygiene concern. and removed 🚨 This issue needs some love. priority: p2 Moderately-important priority. Fix may not be included in next release. priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Feb 25, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 25, 2019
@sduskis sduskis removed 🚨 This issue needs some love. priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 13, 2019
@google-cloud-label-sync google-cloud-label-sync bot added the api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. label Jan 31, 2020
@Temikus
Copy link

Temikus commented Aug 3, 2021

Hey folks, long time no see 👋 I'm now on the other side of the screen and my new company is trying to use Cloud Profiler to better understand our memory consumption.

Unfortunately now we have a P99 HTTP-client latency of 1H in all our tracing, which throws off our app monitoring. Which makes this a pretty serious show-stopper.

Is it possible to address at all? It's a shame it's been open for 4+ years at P2 :(

@Temikus
Copy link

Temikus commented Aug 3, 2021

/CC @JustinBeckwith - hey Justin, sorry to bother you but is it possible to prioritise?

@Temikus
Copy link

Temikus commented Aug 4, 2021

Alright, another try. @fhinkel ?

@JustinBeckwith
Copy link
Contributor

Heh, hey @Temikus :) This repository is entirely owned by @nolanmar511 and the profiler team. Will ping internally.

@Temikus
Copy link

Temikus commented Aug 4, 2021

Legend! Thanks ❤️

@Temikus
Copy link

Temikus commented Aug 20, 2021

Sorry folks, but we’ll be switching away from GCP profiler due to our metrics being skewed by this bug and Datadog offering a much better profiler experience.

You can deprioritise this back if you prefer.

@ckdarby
Copy link

ckdarby commented Nov 23, 2021

Coming here to echo what @Temikus has said. Kind of unacceptable this hasn't been solved yet because it messes with all p99 data.

CC @nolanmar511

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: cloudprofiler Issues related to the googleapis/cloud-profiler-nodejs API. type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

6 participants