Use minified JSON format in ChangeProp
Closed, ResolvedPublic

Halfak created this task.Feb 9 2017, 3:54 PM
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptFeb 9 2017, 3:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Task desc? Context?

I'm going to do this once the deployment is done (hopefully today). For context see T155931: Minify json responses

I'm sorry but Given the rate of requests, it'll be beneficial to both mediawiki nodes and ores nodes. is not satisfactory. Furthermore, the linked PR provides zero additional information.

@mobrovac, we're working on this. Don't sweat it. Please feel free to remove ChangeProp if you are uncomfortable having this on your workboard.

I am asking because you tagging ChangeProp makes me think that you plan to change something in the corresponding rules config and I want to know what is this about (which for some reason is not easy to uncover). If that is the case, please provide the context, and if it isn't, feel free to remove the tag.

Change 337035 had a related patch set uploaded (by Ladsgroup):
Make ORES return minified responses

https://gerrit.wikimedia.org/r/337035

Ladsgroup moved this task from Active to Review on the Scoring-platform-team (Current) board.

Neither of these is going to get merged until we get enough context to understand the changes and their consequences. My repeated requests for context were actively ignored.

@Ladsgroup Em, there's literally no difference between the responses. They're byte-to-byte identical.

Without format=json, response is:

{
  "421063984": {
    "damaging": {
      "prediction": false,
      "probability": {
        "false": 0.9951822864283945,
        "true": 0.004817713571605508
      }
    }
  }
}

With that, it is:

{"421063984": {"damaging": {"prediction": false, "probability": {"false": 0.9951822864283945, "true": 0.004817713571605508}}}}

It reduces response size to its 75% so we would have less I/O both in CP nodes and ORES nodes.
I must note that currently, production and beta cluster is broken and responded with minfied version regardless but that's going to change (tracked in T157721: Investigate default JSON minification behavior in production)

Pchelolo added a comment.EditedFeb 10 2017, 5:16 PM

@Ladsgroup Kk, then a question is whether we want to have pretty JSON at all? Normally in APIs pretty JSON is not the default, at least I've never seen any APIs that return pretty JSON by default. Even special parameters to return pretty JSON are quite rear, since presentation is the client's responsibility.

Another thing is if you're minifying the JSON, why do we leave the unnecessary spaces?

Another idea, specific to CP and preaching - you could add support for HEAD requests and return no content at all, right now we don't use the response.

Halfak added a comment.EditedFeb 10 2017, 5:19 PM

@Pchelolo, we've discussed this and decided that we do want pretty by default. I don't see how this is a changeprop concern

at least I've never seen any APIs that return pretty JSON by default.

The mediawiki API returns pretty JSON by default

Another idea, specific to CP and preaching - you could add support for HEAD requests and return no content at all, right now we don't use the response.

Please file a task if you'd like us to do that and we'll triage it.

@Ladsgroup Kk, then a question is whether we want to have pretty JSON at all? Normally in APIs pretty JSON is not the default, at least I've never seen any APIs that return pretty JSON by default. Even special parameters to return pretty JSON are quite rear, since presentation is the client's responsibility.

I was actually in favor of having minified as default but @Halfak was against it. If you read the PR and the phab card, you can find his line of reasoning.

Another thing is if you're minifying the JSON, why do we leave the unnecessary spaces?

This is the way internal flask minified works. I have no idea why it's that stupid.

Another idea, specific to CP and preaching - you could add support for HEAD requests and return no content at all, right now we don't use the response.

Great idea, I will work on it. Please make a phab card.

Cool! I'll schedule some work on our end to leverage that.

Can we merge this for and then remove it later. With fixing the default behavior now the I/O is going up again. here's the graph. I would love ot work on sending using head method as well if you don't mind.

Change 338389 had a related patch set uploaded (by Ppchelko):
Config: use minified JSON in ORES response

https://gerrit.wikimedia.org/r/338389

Done. Will deploy Monday.

I would love ot work on sending using head method as well if you don't mind.

With all the Edit-Review-Improvements-ReviewStream project ideas it might not be the best idea to switch to HEAD right now, since the response will most likely be used to build the review stream contents. Let's first get clarity on that project before going to HEAD?

Oh, sorry, I've made a new one, didn't look that you've already done it. Anyway, will deploy on Mon.

Change 338389 abandoned by Ppchelko:
Config: use minified JSON in ORES response

Reason:
https://gerrit.wikimedia.org/r/#/c/337035/

https://gerrit.wikimedia.org/r/338389

Change 337035 merged by Ppchelko:
Make ORES return minified responses

https://gerrit.wikimedia.org/r/337035

Mentioned in SAL (#wikimedia-operations) [2017-02-21T18:35:49Z] <ppchelko@tin> Started deploy [changeprop/deploy@4706f9d]: Change-Prop: Make ORES return minified responses T157693

Mentioned in SAL (#wikimedia-operations) [2017-02-21T18:36:44Z] <ppchelko@tin> Finished deploy [changeprop/deploy@4706f9d]: Change-Prop: Make ORES return minified responses T157693 (duration: 00m 55s)

Pchelolo closed this task as Resolved.Feb 21 2017, 6:41 PM

Change deployed. Resolving.