Closed Bug 709325 Opened 8 years ago Closed 8 years ago

Log records JSON without newlines

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: gps, Unassigned)

Details

(Whiteboard: [fixed in services])

Attachments

(1 file)

Currently, the string representation of records contains newlines. There are a few consumers not smart enough to escape the newlines as a literal "\n". This means that certain activities (like parsing) are more difficult than they should be because records span multiple lines.

This seems to be the only place in the Sync ecosystem where newlines are introduced in strings that are logged. I propose removing the newlines from the toString() representation. The downside is the formatted JSON isn't as pretty. But, it is still fairly readable. More importantly, if you naively `|sort` multiple Sync logs together, you get all the data and don't have orphaned lines.

I've tested the attached patch and all tests pass.
Attachment #580554 - Flags: review?(rnewman)
Comment on attachment 580554 [details] [diff] [review]
Remove newlines from record serialization

Review of attachment 580554 [details] [diff] [review]:
-----------------------------------------------------------------

Eh, sure, why not. If we're stuck with crappy textual logging formats, might as well make them easier to process.

r=me with nits.

::: services/sync/modules/record.js
@@ +117,5 @@
>        "index: " + this.sortindex,
>        "modified: " + this.modified,
>        "ttl: " + this.ttl,
>        "payload: " + JSON.stringify(this.payload)
> +    ].join("  ") + " }",

If you're touching these lines, please rewrite the whole function to:

* use string concatenation instead of allocating an array and using join. (It's not even using JSON.stringify to tackle escaping, so this is just labs laziness!)
* call it toString, not WBORec_toString, while you're touching it
* make it use { return } rather than short syntax.

Same applies to CryptoWrap.
Attachment #580554 - Flags: review?(rnewman) → review+
Pushed to s-c with style changes: https://hg.mozilla.org/services/services-central/rev/3bdbd5ea6617
Whiteboard: [fixed in services]
Pushed to m-c: https://hg.mozilla.org/mozilla-central/rev/3bdbd5ea6617
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.