Closed Bug 1303225 Opened 9 years ago Closed 7 years ago

Allow taskcluster-stats-collector to run without taskcluster credentials

Categories

(Taskcluster :: Operations and Service Requests, task)

Product:
Taskcluster
Component:
Operations and Service Requests
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: moraveyo, Mentored)

Details

(Keywords: good-first-bug)

It's useful to run taskcluster-stats-collector without credentials when doing development (so NODE_ENV=development), so that it doesn't send metrics to SignalFx. And this *mostly* works. However, every time the taskcluster-lib-metrics wakes up and tries to send statistics, it fails and logs a big old error message. It would be better to simply disable logging in the development case, by adding a new configuration option that's set to true for development in config.yml, and that passes "mock: true" to the monitor setup function. It would probably be good to log a warning message in this case, just so that we don't accidentally turn this on in production.
Hi can I work on this?
It looks like you've gotten a start on bug 1438852. We have a bunch of applicants looking for good first bugs, so please focus on that one for the moment. When you're finished, if this is still un-claimed, you are welcome to work on it!
Can I try? Would appreciate some clue on where to start.
Hi, can I give it a try?
Hi Elena, Shreya. We are, indeed, running low on good-first-bugs. Elena was here first, on this bug. Perhaps bug 1437015 is a good alternative, Shreya? Elena, the repository is https://github.com/taskcluster/taskcluster-stats-collector. The first step would be to try running the tests for that service locally, and when you have that working, try running the service itself and see the errors I mentioned in comment 0. Then we can get to work on stopping those errors :)
Assignee: nobody → shreya99oak
Oops!
Assignee: shreya99oak → moraveyo
Elena, how is this going?
I've been stuck for a while making it compile until I updated my node.js and all the dependencies. Now it's compiling OK. For now I'm a little lost in docs on how to obtain 'accessToken' for user-config.yml for tests to run. Also 'yarn test' gives me a bunch of eslint errors.
Yeah, this needs at least node 7. You can often figure that out by looking at package.json's `engines` section. As for the accessToken -- the idea of the bug is to fix things so you *don't* need that, so let's leave that un-set for the moment. I deleted my user-config.yml and re-rand the tests, and one failed, so a good place to start would be to skip that test when the pulse credentials aren't available. 1) TaskListener listens: AssertionError: Need Pulse credentials! at new TaskListener (lib/listener.js:57:5) at _callee$ (.test/listener_test.js:55:24) at tryCatch (node_modules/regenerator-runtime/runtime.js:63:40) at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:337:22) at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:96:21) at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30) at node_modules/babel-runtime/helpers/asyncToGenerator.js:28:13 Note that some of the other tests already skip when the credentials are not available: SignalFxRest timeserieswindow - throws an error for a nonexistent metric - returns a list of (timestamp, value) pairs for a demo metric so that might serve as a good example. Regarding `yarn test` -- that's unexpected! I just removed node_modules and reinstalled everything, and re-ran yarn test without any eslint issues. Can you paste the errors into the bug, if it's not 1000's of lines?
eslint error example 1) eslint should have no errors in src/clock.js: AssertionError [ERR_ASSERTION]: ImportDeclaration should appear when the mode is ES6 and in the module context. + expected - actual -false +true at Referencer.ImportDeclaration (node_modules/escope/lib/referencer.js:591:34) at Referencer.Visitor.visit (node_modules/esrecurse/esrecurse.js:104:34) at Referencer.Visitor.visitChildren (node_modules/esrecurse/esrecurse.js:83:38) at Referencer.Program (node_modules/escope/lib/referencer.js:419:18) at Referencer.Visitor.visit (node_modules/esrecurse/esrecurse.js:104:34) at Object.analyze (node_modules/escope/lib/index.js:153:16) at EventEmitter.module.exports.api.verify (node_modules/eslint/lib/eslint.js:850:35) at processText (node_modules/eslint/lib/cli-engine.js:263:31) at processFile (node_modules/eslint/lib/cli-engine.js:298:18) at executeOnFile (node_modules/eslint/lib/cli-engine.js:682:23) at node_modules/eslint/lib/cli-engine.js:719:13 at Array.forEach (<anonymous>) at CLIEngine.executeOnFiles (node_modules/eslint/lib/cli-engine.js:718:18) at Context.<anonymous> (node_modules/mocha-eslint/index.js:31:22)
Without user-config.yml I also have this: 14) SignalFxRest timeserieswindow throws an error for a nonexistent metric: TypeError: Cannot read property 'timeserieswindow' of undefined at _callee2$ (.test/signalfx_test.js:67:35) at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40) at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:296:22) at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21) at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30) at node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14 at new Promise (<anonymous>) at new F (node_modules/core-js/library/modules/_export.js:35:28) at Context.<anonymous> (node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12) 15) SignalFxRest timeserieswindow returns a list of (timestamp, value) pairs for a demo metric: TypeError: Cannot read property 'timeserieswindow' of undefined at _callee3$ (.test/signalfx_test.js:94:35) at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40) at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:296:22) at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21) at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30) at node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14 at new Promise (<anonymous>) at new F (node_modules/core-js/library/modules/_export.js:35:28) at Context.<anonymous> (node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12)
Hm, that is definitely weird. It looks like a bug in eslint or something! Try this: rm -rf node_modules node --version yarn yarn test and let's see what the output is. I suspect the first line might make the difference -- it should reset all of the installed packages.
node is v8.9.4 and yarn is v1.5.1 and gives me that: error taskcluster-stats-collector@1.0.0: The engine "node" is incompatible with this module. Expected version "7.10.1". error taskcluster-stats-collector@1.0.0: The engine "yarn" is incompatible with this module. Expected version "0.21.3".
Hm, I can reproduce *that* issue. The service itself works with a newer node, but newer versions of yarn will check the node and yarn versions. We had a "spring cleaning" project a while back that upgraded all of our services to use Node 8 and the latest yarn. It looks like we missed this service! Do you want to try following the instructions in https://public.etherpad-mozilla.org/p/tc-spring-cleaning-fall-2017 to do that spring-cleaning now? That, alone, will make a good PR. Then you can use the versions of node and yarn that you have installed.
Should I make spring-cleaning PR inside this bug, or file another one?
Associated with this bug is OK. It will still count as a separate contribution :)
When making spring-cleaning, after removing everything about babel (par.8-10 from https://public.etherpad-mozilla.org/p/tc-spring-cleaning-fall-2017) I get the error message: /home/lemur/taskcluster-stats-collector/test/clock_test.js:3 import assume from 'assume'; ^^^^^^ SyntaxError: Unexpected token import I think I found some info about that: https://stackoverflow.com/questions/39436322/node-js-syntaxerror-unexpected-token-import/39436379 https://github.com/nodejs/help/issues/53 The `import` is not supported without babel.
I see in another projects `require` is used, not `import`, for example https://github.com/taskcluster/taskcluster-auth/blob/master/test/containers_test.js Should I do the same here?
Yes please. I'm sorry, I thought there was a note about that in the etherpad. Please add one :)
I've made a PR, work in progress. Could you please have look? I wonder am I going the right way at all... Done changing `import/export` to `require` and `module.exports`. (That's in the PR.) But then found the note "consider using https://github.com/standard-things/esm instead of switching completely to require". Trying to make it this way now, no success yet.
We aren't using esm in any of the other services, so I don't think we want to do so here, either. We'll start using import/export once Node supports them natively.
Hm, that failed when I deployed it, with Mar 14 19:17:49 taskcluster-stats-collector app/run.1: module.js:549 Mar 14 19:17:49 taskcluster-stats-collector app/run.1: throw err; Mar 14 19:17:49 taskcluster-stats-collector app/run.1: ^ Mar 14 19:17:49 taskcluster-stats-collector app/run.1: Error: Cannot find module 'requestretry' Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Function.Module._resolveFilename (module.js:547:15) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Function.Module._load (module.js:474:25) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module.require (module.js:596:17) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at require (internal/module.js:11:18) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Object.<anonymous> (/app/src/signalfx-rest.js:1:79) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module._compile (module.js:652:30) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Object.Module._extensions..js (module.js:663:10) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module.load (module.js:565:32) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at tryModuleLoad (module.js:505:12) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Function.Module._load (module.js:497:3) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module.require (module.js:596:17) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at require (internal/module.js:11:18) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Object.<anonymous> (/app/src/main.js:11:22) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module._compile (module.js:652:30) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Object.Module._extensions..js (module.js:663:10) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Module.load (module.js:565:32) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at tryModuleLoad (module.js:505:12) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Function.Module._load (module.js:497:3) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at Function.Module.runMain (module.js:693:10) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at startup (bootstrap_node.js:188:16) Mar 14 19:17:49 taskcluster-stats-collector app/run.1: at bootstrap_node.js:609:3 Mar 14 19:17:49 taskcluster-stats-collector heroku/run.1: Process exited with status 1 Mar 14 19:17:49 taskcluster-stats-collector heroku/run.1: State changed from up to crashed
Ah, it looks like requestretry was accidentally added as a devDependency (-D) instead of a dependency.
OK, fixed that, and your spring-cleaned version is deployed. Nice work!
Thanks! I'm starting with the bug itself. Could you please again take a look at WIP PR: https://github.com/taskcluster/taskcluster-stats-collector/pull/34
SignalFx tests without credentials didn't skip on my machine, so I started with that. After some research, found out that this.skip() works inside tests, but not inside suites (gives an error "not a function"). I believe it's somehow connected to https://github.com/mochajs/mocha/issues/2683 Tried to change every arrow function to simple one (https://github.com/mochajs/old-mochajs-site/pull/14#issuecomment-139129827), but it didn't help. So I moved skipping on credentials check to individual tests - now it works for me.
https://github.com/taskcluster/taskcluster-stats-collector/pull/34 addressed getting tests to run, but the issue to fix here is that running with NODE_ENV=development, it has errors related to missing taskcluster credentials. Let's make that not happen.
So, the tests run fine now: (v8.10.0) dustin@lamport ~/p/taskcluster-stats-collector [master*] $ yarn test yarn run v1.5.1 $ yarn lint $ eslint src/*.js test/*.js $ mocha test/*_test.js Clock msec ✓ returns the current time setTimeout ✓ sets a timeout (101ms) ✓ handles errors ✓ handles sync errors ✓ handles async errors periodically ✓ runs a function periodically eb calculate ✓ calculates error budget of 1 when SLO is always 1 ✓ calculates error budget of 0 when SLO is 0 for an hour ✓ calculates error budget of about 0.48 when SLO is 0 for a quarter-hour run ✓ calculates datapoints for every hour TaskListener - listens metricstream signalFxMetricStream ✓ rejects invalid resolutions ✓ returns historical data, then "nowLive" ✓ transitions from historical to live data metricLoggerStream ✓ logs datapoints and passes them through signalFxIngester ✓ throws an error on invalid types ✓ ingests datapoints and passes them through multiplexMetricStreams ✓ multiplexes historical data, with missing values carried through from the last occurrence ✓ delays live data consistently ✓ starts when a stream goes live but never produces data ✓ skips missing datapoints collector.pending ✓ nothing happens without input ✓ at startup, unpaired task runs are measured, but that stops once a pair is seen ✓ at startup, unpaired pending tasks are ignored ✓ after a pending/running pair, pending tasks are measured periodically ✓ the measure represents the longest-pending task ✓ long-pending tasks that are not actually pending are found out by check collector.running ✓ nothing happens for pending or running tasks ✓ a resolved task has its run analyzed ✓ deadline-exceeded runs are not timed, but are counted ✓ a run created due to retry, and any runs before it, are ignored SignalFxRest timeserieswindow - throws an error for a nonexistent metric - returns a list of (timestamp, value) pairs for a demo metric 30 passing (5s) 3 pending And no waiting to compile anymore, thanks to the spring cleaning. Nice work on all of that! The problem this bug was orginally filed for happened when using a `user-config.yml` that contains pulse credentials (but not a SignalFx token or Taskcluster credentials, as I don't want to send the data to SignalFx while I'm hacking) to actually *run* the service locally on my laptop: $ DEBUG=* NODE_ENV=development node src/main server --collectors pending However, I just ran this now and it seems to work fine. So some other change in the last 2 years must have fixed this! So, I think this is done -- and you've done a great job fixing the stuff you've fixed :)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: Operations → Operations and Service Requests
You need to log in before you can comment on or make changes to this bug.