\r in metrics API output

I am not sure about the origin of `16/candidate\r` channel, but it seems to work just fine for refreshes.

{{{

$ cat bad_channel.json
{
  "context": [
    {"snap-id": "njObIbGQEaVx1H4nyWxchk1i8opy4h54", "revision": 10, "tracking-channel": "16/candidate\r", "instance-key": "a"}
  ],
  "actions": [
    {"action": "refresh", "snap-id": "njObIbGQEaVx1H4nyWxchk1i8opy4h54", "instance-key": "a"}
  ]
}

$ curl -s -H 'Content-Type: application/json' \
        -H 'Snap-Device-Series: 16' \
        -H 'Snap-Device-Architecture: amd64' \
        'https://api.snapcraft.io/v2/snaps/refresh' \
        -d '@bad_channel.json' \
| jq '.'
{
  "error-list": [],
  "results": [
    {
      "effective-channel": "16/candidate",
      "instance-key": "a",
      "name": "nextcloud",
      "released-at": "2019-05-23T19:23:14.127486+00:00",
      "result": "refresh",
      "snap": {
        "created-at": "2019-05-23T16:15:50.409353+00:00",
        "download": {
          "deltas": [],
          "sha3-384": "e07d888235eae56edcae9c085b1d28dbbd0207a9f98ded18bfe6db79e824f9dc19c6dc308fbbe142d9ea1a00be29f815",
          "size": 252055552,
          "url": "https://api.snapcraft.io/api/v1/snaps/download/njObIbGQEaVx1H4nyWxchk1i8opy4h54_13518.snap"
        },
        "license": "AGPL-3.0+",
        "name": "nextcloud",
        "prices": {},
        "publisher": {
          "display-name": "Nextcloud",
          "id": "lBnslZGp88VvX4EJSAvS2AG2Iryb1MNU",
          "username": "nextcloud",
          "validation": "verified"
        },
        "revision": 13518,
        "snap-id": "njObIbGQEaVx1H4nyWxchk1i8opy4h54",
        "summary": "Nextcloud Server - A safe home for all your data",
        "title": "Nextcloud",
        "type": "app",
        "version": "16.0.1snap1-rc1"
      },
      "snap-id": "njObIbGQEaVx1H4nyWxchk1i8opy4h54"
    }
  ]
}

}}}

It seems that we are "trimming" channels before evaluating operations, but the raw content is recorded in the metrics.

IMO, the trimming is masking deeper problems and should be removed, the refresh operation would fail (client would be fixed if necessary) and no metric would be recorded.

/dev/api/snaps/metrics (for lxd)

{"metrics": [{"status": "OK", "series": [{"values": [1], "name": "3.3/stable"}, {"values": [8], "name": "3.0"}, {"values": [2], "name": "3.0/edge"}, {"values": [14], "name": "latest/stable"}, {"values": [15570], "name": "stable"}, {"values": [58], "name": "candidate"}, {"values": [177], "name": "stable/ubuntu-19.10"}, {"values": [17], "name": "beta"}, {"values": [5], "name": "2.0/stable"}, {"values": [6], "name": "3.0/stable/ubuntu-18.04"}, {"values": [549], "name": "3.0/stable"}, {"values": [6], "name": "stable/ubuntu-18.04"}, {"values": [22135], "name": "stable/ubuntu-19.04"}, {"values": [2], "name": "3.0/candidate"}, {"values": [67625], "name": "stable/ubuntu-18.10"}, {"values": [7], "name": "/stable/ubuntu-19.04"}, {"values": [1], "name": "2.0/candidate"}, {"values": [280], "name": "edge"}, {"values": [152], "name": "3.0/stable/ubuntu-19.04"}, {"values": [1723], "name": "3.0/stable/ubuntu-18.10"}, {"values": [1], "name": "latest/edge"}, {"values": [30], "name": "/stable/ubuntu-18.10"}, {"values": [5], "name": "3.0/stable/ubuntu-19.10"}], "buckets": ["2019-06-06"], "snap_id": "J60k4JY0HppjwOjW8dZdYc8obXKxujRu", "metric_name": "weekly_installed_base_by_channel"}]}

"3.0" and "/stable/ubuntu-19.04" are not valid channel names.

I've disabled https://app-sjg.marketo.com/#SC9748C3 until this is fixed.

I've done some poking around in the db, and the '\r' is in the raw db value for nextcloud, as is the split channels for lxd, so the problem is definitely in the way snapstats miner is generating/storing the aggregates, as opposed to a bad transform when reading them.

I'm looking into fixing the generation, but for bad data already present, it's tricky to know what to do. In this case, on reading the data, we could maybe have a validation of channel names, and just drop bad ones, which should mean the data that is there can be parsed buy the consuming api, although there well be missing data.

Backfilling a fix is tricky, as we don't like to update in cassandra, as it leaves tombstones around.

It's most likely that the bad character in the channel is coming from a client calling the refresh API with it.

On Wed, 12 Jun 2019 at 14:05, William Grant <email address hidden> wrote:

> It's most likely that the bad character in the channel is coming from a
> client calling the refresh API with it.
>

Right, that would make sense. I haven't found anything in the snapstats
code which would explain it, the strings are never parsed, they're opaque.

I would suspect the same for the channel "3.0" and "/stable/ubuntu-18.10",
"/stable/ubuntu-19.04" channel names.

So in that case, the proper fix is channel name validation/sanitization in
snapdevicegw, I think?

> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/1831910
>
> Title:
> \r in metrics API output
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/snapstore/+bug/1831910/+subscriptions
>

--
Simon

I don't think we should concerned about backfilling metrics, once the ingesting part is fixed, the bad tags will fade in a week.

At this point, I am inclined to stop mangling (trimming, adding missing parts, etc) the given bad-channels and fail the refresh request.

FTR, 3.0 is actually a valid channel string, it defaults to 3.0/stable. /stable/ubuntu-{19.04,18.04} is technically not valid, but refresh api actually parses it ok, even it it's actually the wrong value (should be 3.0/stable/ubuntu-19.04)