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

[statsd] Raise ValueError instead of Exception when payload is too large #730

Merged
merged 5 commits into from
Jul 29, 2022
Merged

[statsd] Raise ValueError instead of Exception when payload is too large #730

merged 5 commits into from
Jul 29, 2022

Conversation

mlanicaputo
Copy link
Contributor

What does this PR do?

As noted in #725, statsd.event() raises a general Exception in the event that an event payload is too long. This can lead to error swallowing. This PR changes this error block to raise a ValueError instead.

Description of the Change

The statsd.event() method throws a different Exception (ValueError) in the event that a payload is too large. A test was added to test_statsd.py in order to ensure that the proper error is raised when appropriate.

Alternate Designs

Any exception could theoretically be raised, but ValueError seemed the most appropriate according to Python's guidelines. Alternatively, users of the datadog library could just catch the Exception but that is poor practice.

Possible Drawbacks

Users of the library who currently have code in place to catch a general Exception may have to change their libraries to catch the more specific ValueError.

Verification Process

A new test was added to test_statsd.py in order to ensure the viability of the error change. It tests that a payload that is too large raises the ValueError, and that a payload of allowable length does not raise a ValueError. This can be double checked by running the following from the datadogpy directory: pytest tests/unit/dogstatsd/test_statsd.py -k test_event_payload_error.

Additional Notes

Thank you for being open source!

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

@mlanicaputo mlanicaputo requested review from a team as code owners July 13, 2022 18:04
@mlanicaputo mlanicaputo changed the title Raise ValueError instead of Exception in statsd changelog/Changed : Raise ValueError instead of Exception in statsd Jul 15, 2022
@sgnn7 sgnn7 added the changelog/Changed Changed features results into a major version bump label Jul 28, 2022
@sgnn7 sgnn7 changed the title changelog/Changed : Raise ValueError instead of Exception in statsd statsd: Raise ValueError instead of Exception when payload is too large Jul 28, 2022
@sgnn7 sgnn7 changed the title statsd: Raise ValueError instead of Exception when payload is too large [statsd] Raise ValueError instead of Exception when payload is too large Jul 28, 2022
@sgnn7 sgnn7 added kind/feature-request Feature request related issue severity/minor Minor severity issue labels Jul 28, 2022
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

Hey @mlanicaputo! I left a comment on the PR but it's a minor nit

tests/unit/dogstatsd/test_statsd.py Show resolved Hide resolved
@sgnn7 sgnn7 merged commit 9777317 into DataDog:master Jul 29, 2022
@sgnn7
Copy link
Contributor

sgnn7 commented Jul 29, 2022

@mlanicaputo Merged - thanks for your contribution!

@sgnn7 sgnn7 added this to the 0.45.0 milestone Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Changed Changed features results into a major version bump kind/feature-request Feature request related issue severity/minor Minor severity issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants