-
Notifications
You must be signed in to change notification settings - Fork 20
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
test: do not trigger node bug with fd mapping #94
Conversation
It was broken in f65aef4, which stopped passing the metrics arguments (`runArgs`) to sl-run. With no `--metrics` arguments, no metrics are reported, and the test started to fail.
I take it back, the problems weren't subtle, its just they were obscured by the node bug. Anyhow, I hope to see a green up above RSN. |
logger: config.logger, | ||
}); | ||
var agent = require('../lib/agent'); | ||
var options = { |
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.
It's really really minor, but could this be agentOptions
to make the association more clear?
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.
yes
Before this PR I hadn't considered whether JS's TIL: JavaScript uses Interestingly (to me, at least), Ruby also uses a |
Oh ya.. LGTM, other than a couple very minor style nits. |
👍 Thanks for fixing this @sam-github |
It used to be profiling was only started when StrongOps was started. This caused problems for metrics and other users of the agent, because when agent is no started first, it doesn't behave well. In particular, it doesn't monkey patch anything, causing metrics to be lacking probe metrics, and agent traces and express records not be available.
c39616e
to
4debc61
Compare
Fix bugs in and revealed by unit tests
I was a bit worried about the switch coercion, but I tested, and the docs are explicit, I'm glad it works! |
See nodejs/node#862
Note this is not enough to fix the tests, more subtle problems are causing the test failures for the express records.
/to @rmg this should effect tap, AFAICT.
/cc @bajtos