#haraka

And that was my bad because I edited your version to get rid of the toString() in an attempt to speed it up.

Here's my fastest version:

Note the `key` declared outside of the `for` constructor is required for V8 to optimize it. No idea why.

That gives me now 0m5.425s vs 0m3.356s

Whic is probably close enough.

I thought trim was faster than substr

Why?

trim has to look at both ends of the string.

substr() can just change the pointer.

that's what I thought to but I thought chrome used v8 and it reflected the performance on node :(

s/to/too/

the jsperf benchmark would prefer trim > substr

OK so trim() is a tiny tiny fraction faster.

doing toString() before the regex.test brings me down to 0m5.179s

baudehlo: Ok - that's the fastest I've seen it. 40.4% slower here

I wrote a versoin that does no search/replace and the fastest I can get it is ~4s

so we're cutting into diminishing returns now.

With the toString() outside it's 37% slower

*nod* - this isn't exactly real world.

yeah, it's always going to be slower. The non-replace version is the absolute fastest it could ever be.

But it's way better than logfmt.stringify() and requiring an external module.

yeah

Which was my bugbear with it.

I don't see why it needed it

Cool, so I'll update the PR with baudehlo's version?

and remove logfmt

Sure - you want to gist your final version baudehlo?

I'm glad we could get this sorted :)

Sure:

baudehlo: ta - I just wanted to make sure mine was the same (it was)

I tried changing the regexp to v.quote() but it's only in firefox's JS engine.

I wonder if str.search(regexp) is faster than regexp.test(str)

The only issue I can see with this is that helo="[1.2.3.4]" as we have now is going to become helo="\[1.2.3.4\]"

Isn't it?

No - my bad.

no, the square brackets are a char class.

fwiw str.trimRight() seems fractionally faster.

Hang on a sec.

I still can't believe you can do:

> "foo".fontcolor("red");

'<font color="red">foo</font>'

LOL

Why is it changing relay=Y to relay="Y"

Oh - because it's going to require it in that format as it's basically turning the log into JSON.

Shouldn't we keep that the same as it is now?

I'm thinking that if people have log grokkers written, then this is going to break them potentially.

With logfmt mode enabled shouldn't it be relay=true or relay=false ?

anosh: ??

it *should* probably be an actual Boolean

_smf_: I'm fine with whatever is easier to keep it simple for both formats

baudehlo: what do you think? I don't think we should be changing the formatting of the logs, especially in a point release?

could update all the Y's to true then in `default` it'll be relay=true instead of relay=Y too but idk what the reason for Y was

I'm fine with keeping it Y if it's easier

As long as we're aware what to search for

anosh: but relay=Y isn't going to work for you is it?

It will

You'll need it as relay="Y"

Oh

You see what I mean.

We don't NEED the quotation marks, it shouldn't be doing it if it's one word

relay=Y should work

relay="Very true"

if it has a space or an equals, then it should have the quotation

I'll update baudehlo's snipped it do that since it's just removing the quotation marks from the last template literal in his pastebin

snippet*

anosh: OK - yeah

It's fine then.

ultimatt: logger doesn't accept `protocol` as a level from log.ini, it accepts `logprotocol`.

then that's a bug, because that's what this block in logger is for:

for (var le in logger.levels) {

logger['LOG' + le] = logger.levels[le];

}

and if true, that means the example code and docs in master right now is broken

That's when it sets the log* values on logger, you mean in set_loglevel? it does `logger.loglevel = logger[level.toUpperCase()];`?

yep, that's there so that LOGDEBUG and DEBUG both return values

DEBUG would return a value if it was in logger.levels, not in logger, that for loop set's LOGDEBUG on logger

I can update it to create LOGDEBUG in logger.levels instead and have it check there?

DEBUG doesn't exist on logger itself

I'm looking at it again, I may have read it too fast

:P

Any why is level=protocol the default given that the default was LOGINFO before?

the default wasn't LOGINFO before.

Well, at least not before the tls-ini PR

the default was LOGPROTOCL

maybe at some point in the past it was...

I think LOGINFO is a better default, but I didn't want *that* issue being an objection to my PR

It's been default since 2012

Hmmm - I never noticed that before.

It used to be LOGDEBUG but got changed to LOGPROTOCOL

Not the best default for sure.

anosh is right

anosh, this fixes it right? https://gist.github.com/msimerson/668c46adfb7eb...

and lets us use INFO instead of LOGINFO in the config files?

I think if you just update the initial for loop to add to logger.levels then in set_loglevel, you do logger.loglevel = logger.levels[level.toUpperCase()];, it's be easier to understand

new_level seems too weird

idk how logger.LOG* is being used everywhere else besides logger.js though

yeah okay, that works too

yeah

it's just if anything is depending on the logger.LOG*, then those plugins might break

ultimatt: I'd say the introduction of log.ini is a good as time as any to default the logging to LOGINFO or LOGNOTICE given that this will only affect new installations.

I'm going to go see how the syslog plugin works quickly

_smf_: I agree.

+1

Plus config/loglevel uses LOG*

rather than just DEBUG, PROTOCOL etc.

right, but we want it to work both ways. :)

Yeah - that's what I'm talking about.

yeah

[13Haraka] 15msimerson opened pull request #2056: support INFO *and* LOGINFO as config settings (06master...06log-defaults) 02https://github.com/haraka/Haraka/pull/2056

ultimatt: `logger.levels[`LOG${le}`] = le;` should be `logger.levels[`LOG${le}`] = logger.levels[le];`?

because `le` is just a key?

yep

not finished, I'm writing a test for it now.

so I'd have caught that in a few more minutes. :)

ah

lmao

it's cool

I'm just too eager

No - to be fair he should have marked it WIP.

to be fair, the CI tests haven't finished running yet.

And that is the problem with CI. It makes people lazy.

right, because lazy people write tests. I remember seeing that in Best Practices manuals everywhere

No - it's because some people don't bother doing their own extensive tests and rely on the CI tests being right instead.

And I got caught in that trap myself.

and people also like to assume that they can think of every use case and that they've tested them all and so they don't need tests. Maybe we need manual tests, CI tests, AND human code reviews and all are good things.

okay: https://github.com/haraka/Haraka/pull/2056 ready for review

Yeah - I'm not disagreeing. I'm merely pointing out that you submitted a half-baked PR without marking it WIP and then used the CI tests not finishing as an excuse.

The TLS issue is a case in point - there was no review and the CI didn't pick up the problem.

the alleged problem

Yeah - reported by a user - on here - that when they reverted from HEAD to 2.8.14 - it suddenly started working.

sure, and there's likely an issue.

but, it's HEAD, and the way we get reports like that is merging and letting the brave folks who run from HEAD do so

and the human code reviews of the TLS PR yesterday failed to catch what anosh bumped into today and what this PR fixes.

ahem, the logging PR

I misread the code and assumed DEBUG was a valid shorthand for LOGDEBUG, and conveniently, so did everyone else.

we're humans, it happens.

Right - so next time, don't revert my code then. And I will extend you the same grace.

then don't break stuff, brag about not testing it, and then release it to NPM

I didn't brag about testing it. I made sure the CI tests passed (which was my error) and I had no way of testing it as I don't use the plugins.

IOW, you didn't test it at all.

And I'm happy to admit I was wrong. But don't go self-merging stuff and think I'm not going to pick you up on it when you introduce the same type of bug.

the plugins aren't hard to use. I've regularly enable plugins I don't use to test they work (example: DCC) after I whack at them.

Right - because DCC really is the same class..

Hey Steve, how long was the TLS PR open before I self merged?