Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-6376: Document skipped records metrics changes #4922

Merged
merged 2 commits into from
Apr 24, 2018
Merged

KAFKA-6376: Document skipped records metrics changes #4922

merged 2 commits into from
Apr 24, 2018

Conversation

vvcephei
Copy link
Contributor

Document the metrics changes in ed51b2c .

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei
Copy link
Contributor Author

@guozhangwang @mjsax @bbejeck,
Please let me know if these docs changes are ok.

Copy link
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit comment, otherwise LGTM.

@@ -101,6 +101,16 @@ <h1>Upgrade Guide and API Changes</h1>

<!-- TODO: verify release verion and update `id` and `href` attributes (also at other places that link to this headline) -->
<h3><a id="streams_api_changes_120" href="#streams_api_changes_120">Streams API changes in 1.2.0</a></h3>
<p>
We have removed the <code>skippedDueToDeserializationError-rate</code> and <code>skippedDueToDeserializationError-total</code> metrics.
Deserialization errors, and all other causes of record skipping, are now accounted for in the pre-existing metrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we list the possible reasons of skipped records here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I considered doing that but was overcome by laziness. ;) I'll add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working my way though this, but just had this thought... if we don't want people to depend on record skipping for correctness, is there an argument that we shouldn't document the behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "correctness" here is really dependent on users app case-by-case: if I do not expect ANY skipped records, then I will monitor on this metric > 0, if I do expect, that sometimes there are null keys from the source topic that will be skipped, then I will probably not monitor on this metric or just monitor on it not larger than a very big value indicating spiky anomaly.

Copy link
Contributor

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vvcephei, LGTM

@vvcephei
Copy link
Contributor Author

@guozhangwang I've added the reasons.

@guozhangwang guozhangwang merged commit 12a0f46 into apache:trunk Apr 24, 2018
jeqo pushed a commit to jeqo/kafka that referenced this pull request May 2, 2018
Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants