Skip to content
This repository was archived by the owner on Feb 18, 2021. It is now read-only.

Log willSample prepublish replacer #211

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Log willSample prepublish replacer #211

wants to merge 16 commits into from

Conversation

rf
Copy link
Contributor

@rf rf commented Jan 8, 2016

This requires an updated larch so it isn't ready to land yet, but it works.

Wrote a simple script to insert willSample checks before log lines. I messed around with using some macro systems (sweet.js and the C preprocessor) but found that neither would work. The replacer script is an inelegant solution but it's short and it works.

r: @Raynos @jcorbin

@@ -78,7 +78,8 @@
"test-ci": "npm run check-licence && npm run lint -s && npm run cover",
"test-repeat": "NODE_DEBUG=autobahn node test/index.js | FORCE_COLOR=1 tap-spec; while [ $? -eq 0 ]; do NODE_DEBUG=autobahn node test/index.js | FORCE_COLOR=1 tap-spec; done;",
"test-cover": "istanbul cover test/index.js",
"view-cover": "opn ./coverage/index.html"
"view-cover": "opn ./coverage/index.html",
"prepublish": "git ls-files | grep '.js$' | grep -v 'bin/' | grep -v test | grep -v replacer.js | xargs node replacer.js"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the same prepublish in tchannel :)

Which means tchannel is coupled to larch... which means... let's just figure it out.

@Raynos
Copy link
Contributor

Raynos commented Jan 8, 2016

This seems reasonable.

I want to try running it and looking at a diff of the pre-process vs post-process.

Also we need to figure out what to do in terms of workflow, prepublish might have to copy into a "build/*" folder and we should change the package.json to import from there.

Then we can .gitignore build/*

@Raynos
Copy link
Contributor

Raynos commented Jan 9, 2016

tests fail lal.

@rf
Copy link
Contributor Author

rf commented Jan 9, 2016

Changed this to use publish.sh. It's real ugly but it runs the replacer, makes a temp commit, creates the package, then resets git.

git ls-files | grep '.js$' | grep -v 'bin/' | grep -v test | grep -v replacer.js | xargs node replacer.js

# need a temp commit because git archive uses HEAD
git commit -a -m 'temp commit'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run the test suite here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'll fail bc DebugLotron everywhere (including in TChannel's test cluster or whatever) which doesn't have the right interface

@Raynos
Copy link
Contributor

Raynos commented Jan 11, 2016

we need to fix debug logtron.

return msgMatches[1];
}

// Inserts an if (logger.willSample('level')) before a logsite for a particular
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update comment: willSample('level', 'msg')

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

Successfully merging this pull request may close these issues.

3 participants