allow for empty X-Parent #820
Conversation
also tighten up optionality of params
carldunham
force-pushed
the
cd/GQL-3068/accept-passed-trace-ids
branch
from
121cca0
to
d1a339b
Compare
tests/unit/core_tests.py
Outdated
|
|
||
| def test_from_upstream_handles_no_parent(self): | ||
| span = TraceInfo.from_upstream(1, None, 3, None, None) | ||
| self.assertIsNone(span.parent_id) |
There was a problem hiding this comment.
Just a suggestion: it might be nice to include a bit of context here with a brief comment.
In my experience this is the kind of test that people come back to later on and think "why are we testing this? can this even happen in real code? of course there's always a parent span because of [xyz reason that doesn't apply in your case]" and end up breaking things. Since this is testing a non-obvious but important scenario, you could add a line like
def test_from_upstream_handles_no_parent(self):
# Ensure that TraceInfo can be constructed with no parent span. This can happen when
# the root span is created by this service.
...(adjust to match the actual reason)
You bet! So this is coming up where we have bp.go services and a bp.py service. We can see the In debugging the cause of this, it was noticed that the code would silently fail when no So this PR aligns what is required and what is optional, so that it will work as expected. |
tests/unit/core_tests.py
Outdated
| """ | ||
| Verify that `parent_id` can be left out without raising an exception. | ||
| """ |
There was a problem hiding this comment.
nit (no need to fix): I would suggest following PEP257 when possible since it produces better output from pydoc (and other tools that parse docstrings): https://peps.python.org/pep-0257/#one-line-docstrings
also tighten up optionality of params
💸 TL;DR
The
X-Parentheader could be empty, and will be for the root span. This PR allows for that.📜 Details
🧪 Testing Steps / Validation
✅ Checks