fix: Pad milliseconds with three zeroes

[bmaupin]

Hi!

You happen to have the only fork of glossy that's been published to NPM, so I'm hoping you'd be willing to merge this fix and publish a new release.

The milliseconds were padded only with two zeroes, e.g.

<131>1 2021-09-27T15:52:35.35-07:00 localhost test-graylog-syslog 103065 - - error: Test message

Unfortunately Graylog parses 15:52:35.35 as 15:52:35.350 instead of 15:52:35.035, so this fixes that.

Here's a related issue with more details: winstonjs/winston-syslog#139

Thanks!

[krisreeves-stripe]

Hi there. I caught this in my inbox but can't look at it until later. I had a frustrating time with the upstream in the past, so I'm happy to help here. However, I've been very distant from this code and don't understand the full ramifications of a change here. It seems absolutely correct that if the value being produced is milliseconds, then "35 milliseconds" should be ".035"; but I'll have to re-read the code and associated specs to determine whether the decimal is supposed to be two digits. In other words, is the correct output here ".03" or ".035"?

Since you've been running this down, if you have any reasoning or references here to help me arrive at an answer easier, I'd appreciate it :)

[bmaupin]

In other words, is the correct output here ".03" or ".035"?

Great question!

If I'm reading the spec correctly, it appears to allow anywhere from 1 - 6 digits of precision after the decimal point (based on the rule format from https://datatracker.ietf.org/doc/html/rfc5234#section-3.6):

TIME-SECFRAC = "." 1*6DIGIT

(https://datatracker.ietf.org/doc/html/rfc5424#section-6)

While two digits of precision is probably valid, three seems to be more common from what I've seen in other libraries that support RFC 5424 such as log4j2. There's even an example in the spec for milliseconds:

Example 3

   2003-10-11T22:14:15.003Z

This represents 11 October 2003 at 10:14:15pm, 3 milliseconds into
the next second. The timestamp is in UTC. The timestamp provides
millisecond resolution. The creator may have actually had a better
resolution, but providing just three digits for the fractional part
of a second does not tell us.

(https://datatracker.ietf.org/doc/html/rfc5424#section-6.2.3.1)

Interestingly enough, I also just ran across this in the spec which addresses this specific bug:

The TIMESTAMP described in Section 6.2.3 supports fractional seconds.
This provides grounds for a very common coding error, where leading
zeros are removed from the fractional seconds. For example, the
TIMESTAMP "2003-10-11T22:13:14.003" may be erroneously written as
"2003-10-11T22:13:14.3". This would indicate 300 milliseconds
instead of the 3 milliseconds actually meant.

(https://datatracker.ietf.org/doc/html/rfc5424#appendix-A.4)

Hope that helps!

[myndzi]

So, weirdly, RFC3339 (what the function says it generates) says time-secfrac = "." 1*DIGIT, but the same RFC contains an example with two-digit fractional time. I'm not sure I've ever actually seen two-digit fractional time, so this seems fine as-is. The RFC for Syslog that you reference seems to suggest it's fine, and I see no reason to reject it. I can't remember anymore if Syslog format is expected to be parsed by the syslog server you send to or what, which is the only thing that'd really give me any concern. However as you note, this is a fork so I'm not really bothered about it at all :)

I would however like to avoid using .padStart(); this library is pretty old IIRC, and I don't think it's worth introducing a "new-ish" (Node > 7.0) feature just for a simple convenience. Would you mind instead either implementing it into the leadZero function, or a secondary supporting function? That way at least if anyone is using it in some old application, we won't break their code with a patch version bump.

Also, if there are no tests here that break on the new code, there should be at least one new test.

[bmaupin]

So, weirdly, RFC3339 (what the function says it generates) says time-secfrac = "." 1*DIGIT, but the same RFC contains an example with two-digit fractional time.

I skimmed RFC3339, but then I saw this in RFC5424, after which point I focused on that RFC instead:

The TIMESTAMP field is a formalized timestamp derived from [RFC3339].

Whereas [RFC3339] makes allowances for multiple syntaxes, this
document imposes further restrictions.

(https://datatracker.ietf.org/doc/html/rfc5424#section-6.2.3)

I would however like to avoid using .padStart(); this library is pretty old IIRC, and I don't think it's worth introducing a "new-ish" (Node > 7.0) feature just for a simple convenience. Would you mind instead either implementing it into the leadZero function, or a secondary supporting function? That way at least if anyone is using it in some old application, we won't break their code with a patch version bump.

.padStart was introduced in Node 8, which has been end of life for almost two years now (2019-12). But it doesn't hurt to be safe.

If it was just me I'd probably just replace the whole thing with .toISOString() instead of reinventing the wheel, but unfortunately that always uses UTC. In theory it would be fine since it represents the exact same time and adheres to the spec; I know Graylog would parse it identically to a local time. But it's probably too drastic, even if it is the documented behaviour of this function (I'm guessing the comment is just out of date):

 *  Generate date in RFC 3339 format. If no date is supplied, the default is
 *  the current time in GMT + 0.

(https://github.com/myndzi/glossy/blob/master/lib/glossy/produce.js#L356-L357)

While going down the rabbit hole I ran across this commit, which I think makes the change in a more backwards-compatible manner (should support Node >= 0.10) and also fixes a couple other minor issues in this same method:

pavkriz@d6293ca

So maybe I'll see if I can rebase that on your repo.

Also, if there are no tests here that break on the new code, there should be at least one new test.

💯

[krisreeves-stripe]

.padStart was introduced in Node 8, which has been end of life for almost two years now (2019-12). But it doesn't hurt to be safe.

Yeah, it's more a sort of economy of work vs expectation for me. Generally speaking if it doesn't take a lot of effort and the library is broadly useful, I try to keep compatibility as far back as possible. It's unclear what the correct semver thing to do would be since this isn't exactly a breaking change in the package's API, but it very much feels like a patch version bump -- and I hate it when things break on me unexpectedly :)

While going down the rabbit hole I ran across this commit, which I think makes the change in a more backwards-compatible manner (should support Node >= 0.10) and also fixes a couple other minor issues in this same method:

Sounds good!

[bmaupin]

It's unclear what the correct semver thing to do would be since this isn't exactly a breaking change in the package's API, but it very much feels like a patch version bump -- and I hate it when things break on me unexpectedly :)

Since this isn't version 1.0 yet, I think there's some flexibility:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

(https://semver.org/#spec-item-4)

But I think see what you mean; on the one hand this is a fix (i.e. "patch") on the other it could be seen as a breaking change (i.e. major). Since it's < 1.0 you could split the difference and bump the minor version 🤷‍♂️ I don't have a strong opinion either way (not that you're asking for it 😄)

[bmaupin]

This is unrelated, but if you'd like I could also make some other quality-of-life improvements as I run across them. Off the top of my head:

• The NPM listing for this package still points to the upstream repo. I think it'd just need to be updated in package.json
• Some of the other things could probably be updated in package.json too, although I don't know if it's worth bothering with. Like updating maintainers or just removing it (sometimes I think no information is better than bad information)
• Since this repo has tests, I could add a basic GitHub Actions setup so that they're run automatically for PRs. I like doing that so right away I know if a change is going to break tests

[krisreeves-stripe]

Sure :) I don't have any real intent to "maintain" this actively as an alternative to the upstream repo, but I'm happy to respond to any requests and keep a published version available, etc. Those sound like reasonable changes.

Edit: I'll be at work for some while yet, but can merge and publish this later

[bmaupin]

Cool, sounds good. I would typically open separate PRs for separate "changes" but the package.json stuff should be very trivial and the GitHub Actions would actually be relevant because it will run the tests for this bug, so if it's okay with you I'll just make more commits to this PR.

[krisreeves-stripe]

Works for me :) Might as well include a patch version bump too, since that'll be necessary to publish the changes anyway

[bmaupin]

Ok, I think I'm all done. I tried to keep the package.json changes to a minimum (by my standards 😆)

[myndzi]

Thanks a lot :) Hopefully this is useful to more folks!

Just published to NPM

[bmaupin]

Awesome, thanks so much!