traceIdFrom and spanIdFrom return safe defaults instead of throwing - #137
i-am-paradox wants to merge 2 commits into
Conversation
…faults instead of throwing Report the FormatException through OTelErrorHandling and return an invalid id, matching the pattern TraceState.put already uses.
|
Code changed underneath this. I resolved one conflict (to your change). |
michaelbushe
left a comment
michaelbushe
left a comment
There was a problem hiding this comment.
After merge resolution, six tests fail, all the ones Robert added in #112 that assert the throw you're removing:
trace_id_test.dart: rejects a correctly sized uppercase hex string
trace_id_test.dart: rejects a signed hex string instead of corrupting the id
trace_id_test.dart: rejects lowercase hex that is not 32 characters
span_id_test.dart: rejects a correctly sized uppercase hex string
span_id_test.dart: rejects a signed hex string instead of corrupting the id
span_id_test.dart: rejects lowercase hex that is not 16 characters
It's all the same test pattern you already used: capture a handler, assert the returned id is invalid, assert one FormatException arrived. The inputs are worth keeping exactly as they are, since each one documents a specific way the old int.tryParse corrupted an id.
Leave traceIdOf and spanIdOf alone. They take raw bytes, still throw ArgumentError, and this PR doesn't touch them.
Two other things while you're in there. Six tests install an OTelErrorHandling handler and only two have addTearDown(OTelErrorHandling.resetToDefault). Without it a failing expectation leaks the capturing handler into every later test in the file, so one real failure becomes a cascade.
Also, this needs a CHANGELOG entry under ### Fixed in 1.0.0-rc.4-wip; it changes behavior callers depend on, and the convention here is to link the PR rather than the issue.
There's also an analyze fix. If you install the git-hooks (See CONTRIBUTING.md), it will be on pre-push.
Thank you.
2 participants
Fixes #132 using the approach you outlined in the issue: report through OTelErrorHandling and return the invalid id, same as TraceState.put does for a bad entry.
I updated the existing throws-expectations in trace_id_test and span_id_test, and added coverage that a handler-installed test actually receives the FormatException, including the wrong-length case. The full suite passes (1175 tests) and analyze is clean.