Count utf-8 bytes in the bytes_sent and bytes_dropped_writer telemetry - #984
dylanpulver wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22c5195158
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| if not is_telemetry and self._telemetry: | ||
| self.bytes_dropped_writer += len(packet) | ||
| self.bytes_dropped_writer += len(packet.encode(self.encoding)) |
There was a problem hiding this comment.
When a payload contains an unpaired surrogate, or self.encoding cannot represent it, _xmit_packet_attempt catches the initial packet.encode(...) exception and returns False, but this line immediately performs the same encoding outside that exception handler. Synchronous metric calls therefore now propagate UnicodeEncodeError; with the background sender, the exception terminates _sender_main_loop before queue.task_done(), so subsequent metrics are never sent and wait_for_pending() can hang indefinitely. Preserve the writer's existing failure containment while calculating the byte count only when encoding succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I want to look a bit more into this as feel this might be a legitimate concern. Overall the change looks good.
_xmit_packet encodes the packet at base.py:1782 and sends encoded_packet, then charges bytes_sent with len(packet) on the unencoded str, so a multibyte payload is under-counted: "city:montréal" puts 31 bytes on the wire and is reported as 30. Same on the two bytes_dropped_writer sites and on the telemetry packet itself, which carries the constant tags. base.py:1648 already does this correctly, from DataDog#927 "fix: report the right number of dropped bytes"; that PR fixed bytes_dropped_queue and left the other four sites. The suite could not see it: assert_equal_telemetry's default expectation is len(<str>) of the same payload, and every explicit bytes_sent= is the same character count, including in the tests that use multibyte input. Those expectations now go through a utf8_len() helper. Fixes DataDog#983 Co-Authored-By: Claude <noreply@anthropic.com>
The writer drops a packet it cannot encode and returns without raising, so counting its bytes must not raise either. _wire_len falls back to the character count, which is what these counters held before this branch. Without it, a payload carrying an unpaired surrogate propagates UnicodeEncodeError out of the send path, which would stop the background sender loop before task_done() and hang wait_for_pending(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M5uTyCU4WcNTsPvGrErDo
1cd7996 to
3acb4bc
Compare
This branch has not been deployed
2 participants
What does this PR do?
Fixes #983. Makes the
bytes_sentandbytes_dropped_writertelemetry counters count bytes.Description of the Change
_xmit_packetencodes the packet atbase.py:1782and sendsencoded_packet, then chargesself.bytes_sent += len(packet)on the unencodedstrat:1791.:1662,:1666and:1734have the same shape.:1648is already byte-correct, from #927 "fix: report the right number of dropped bytes" — that PR fixedbytes_dropped_queueand did not touch the other four. This finishes that job, using the samelen(x.encode(self.encoding))form (at:1791it just uses theencoded_packetalready in scope).The test change is the consequence, not decoration:
assert_equal_telemetrybuilt its default expectation astelemetry_metrics(bytes_sent=len(expected_payload)), the same character count the implementation used, and all 39bytes_sent=len(sites in the file did the same. They now go through autf8_len()helper. 14 existing tests fail against the fixed source until those expectations are corrected — the ones with multibyte payloads.Alternate Designs
Fixing only
:1791(the one path a test can reach easily) and leaving the other three. Rejected: they are the same defect in the same function, and two of the three turned out to be testable after all.Possible Drawbacks
The counters will now report larger numbers for any non-ascii payload. That is the point, but anyone alerting on absolute
datadog.dogstatsd.client.bytes_sentfrom a service with multibyte tags will see a step change.Verification Process
Against a real UDP socket, comparing the datagram received with the value the client then self-reports in
datadog.dogstatsd.client.bytes_sent(master @fac2d5e, Python 3.13.12):env:prod(ascii control)city:montréalregion:東京event("deploy", "done 🚀")service_check(message="café unreachable")4/5 wrong before, 0/5 after; the ascii control passes on both sides.
pytest tests/unit/dogstatsd: 164 passed / 1 skipped on clean master, 167 passed / 1 skipped here (three new tests).flake8 datadogandmypy --config-file mypy.ini datadogboth clean at the tox-pinned versions (7.1.2 / 1.14.1).Mutants: reverting the source with the corrected expectations in place fails 15 tests and nothing else. Fixing only
:1791and leaving the other three leaves exactly the two new counter tests failing — that is what told me:1662and:1734were reachable.Not verified:
:1666, the telemetry-packet-dropped path. I could not construct a test that drops a telemetry packet without also breaking the metric send, so that one line is changed by inspection only, on the strength of being the same expression as the three next to it. I also ran only Python 3.13; no Python 2 path was exercised, and I have not checked how the Agent's telemetry consumer reacts to the counters stepping up.Additional Notes
Written with Claude Code. Not from a production incident: found checking datadogpy's DogStatsD wire handling field by field against
datadog-go, where the byte/character split is where they disagree.