{"action":"created","issue":{"url":"https://api.github.com/repos/prometheus/prometheus/issues/1448","repository_url":"https://api.github.com/repos/prometheus/prometheus","labels_url":"https://api.github.com/repos/prometheus/prometheus/issues/1448/labels{/name}","comments_url":"https://api.github.com/repos/prometheus/prometheus/issues/1448/comments","events_url":"https://api.github.com/repos/prometheus/prometheus/issues/1448/events","html_url":"https://github.com/prometheus/prometheus/pull/1448","id":137965448,"number":1448,"title":"Handle errors caused by data corruption more gracefully","user":{"login":"beorn7","id":5609886,"avatar_url":"https://avatars.githubusercontent.com/u/5609886?v=3","gravatar_id":"","url":"https://api.github.com/users/beorn7","html_url":"https://github.com/beorn7","followers_url":"https://api.github.com/users/beorn7/followers","following_url":"https://api.github.com/users/beorn7/following{/other_user}","gists_url":"https://api.github.com/users/beorn7/gists{/gist_id}","starred_url":"https://api.github.com/users/beorn7/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/beorn7/subscriptions","organizations_url":"https://api.github.com/users/beorn7/orgs","repos_url":"https://api.github.com/users/beorn7/repos","events_url":"https://api.github.com/users/beorn7/events{/privacy}","received_events_url":"https://api.github.com/users/beorn7/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":8,"created_at":"2016-03-02T19:17:08Z","updated_at":"2016-03-09T10:39:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/prometheus/prometheus/pulls/1448","html_url":"https://github.com/prometheus/prometheus/pull/1448","diff_url":"https://github.com/prometheus/prometheus/pull/1448.diff","patch_url":"https://github.com/prometheus/prometheus/pull/1448.patch"},"body":"@juliusv The announced 4th level... (although level 1 is almost done, and level 3 is already removed).\r\nThis fixes https://github.com/prometheus/prometheus/issues/877\r\n\r\nThis requires all the panic calls upon unexpected data to be converted\r\ninto errors returned. This pollute the function signatures quite\r\nlot. Well, this is Go...\r\n\r\nThe ideas behind this are the following:\r\n\r\n- panic only if it's a programming error. Data corruptions happen, and\r\n  they are not programming errors.\r\n\r\n- If we detect a data corruption, we \"quarantine\" the series,\r\n  essentially removing it from the database and putting its data into\r\n  a separate directory for forensics.\r\n\r\n- Failure during writing to a series file is not considered corruption\r\n  automatically. It will call setDirty, though, so that a\r\n  crashrecovery upon the next restart will commence and check for\r\n  that.\r\n\r\n- Series quarantining and setDirty calls are logged and counted in\r\n  metrics, but are hidden from the user of the interfaces in\r\n  interface.go, whith the notable exception of Append(). The reasoning\r\n  is that we treat corruption by removing the corrupted series, i.e. a\r\n  query for it will return no results on its next call anyway, so\r\n  return no results right now. In the case of Append(), we want to\r\n  tell the user that no data has been appended, though.\r\n\r\nMinor side effects:\r\n\r\n- Now consistently using filepath.* instead of path.*.\r\n\r\n- Introduced structured logging where I touched it. This makes things\r\n  less consistent, but a complete change to structured logging would\r\n  be out of scope for this PR."},"comment":{"url":"https://api.github.com/repos/prometheus/prometheus/issues/comments/194233189","html_url":"https://github.com/prometheus/prometheus/pull/1448#issuecomment-194233189","issue_url":"https://api.github.com/repos/prometheus/prometheus/issues/1448","id":194233189,"user":{"login":"beorn7","id":5609886,"avatar_url":"https://avatars.githubusercontent.com/u/5609886?v=3","gravatar_id":"","url":"https://api.github.com/users/beorn7","html_url":"https://github.com/beorn7","followers_url":"https://api.github.com/users/beorn7/followers","following_url":"https://api.github.com/users/beorn7/following{/other_user}","gists_url":"https://api.github.com/users/beorn7/gists{/gist_id}","starred_url":"https://api.github.com/users/beorn7/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/beorn7/subscriptions","organizations_url":"https://api.github.com/users/beorn7/orgs","repos_url":"https://api.github.com/users/beorn7/repos","events_url":"https://api.github.com/users/beorn7/events{/privacy}","received_events_url":"https://api.github.com/users/beorn7/received_events","type":"User","site_admin":false},"created_at":"2016-03-09T10:39:08Z","updated_at":"2016-03-09T10:39:08Z","body":"WRT crashrecovery: It is happening automatically when a server crashes and gets restarted by the init system (runit or whatever users use). To apply a storage forensics tool, we needed to prevent a restart, and that's not what users would like in the general case. (They want their monitoring server to be up ASAP to continue monitoring.) The logging of quarantined series is a bit less critical as the quarantined series with its error message is visible in the `orphaned` directory. So I could be convinced to have that log at debug level. My concern would be that it will be less visible to the naive user what has happened. But then we might not want to optimize for the naive user…\r\n\r\nWRT interface: I consider it bad interface design to return errors that will always be `nil`. Please note that we are not ignoring any errors here. (We are _handling_ them.)\r\n\r\nWRT \"sane default\": The nopIter is not a sane default. It is the truth. The chunk in question is corrupt. We quarantine the series. It is removed from the storage. The appropriate response for iterating over a non-existing series is a nopIter."}}