feat: add customer trace id header - #72
Conversation
|
Code Climate has analyzed commit d24c926 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 97.2% (50% is the threshold). This pull request will bring the total coverage in the repository to 83.7% (0.4% change). View more on Code Climate. |
CodeBooster97
left a comment
CodeBooster97
left a comment
There was a problem hiding this comment.
The user should be able to change the customer_trace_id at anytime.
Can you provide an example that we can share with HMG how to set the customer_trace_id after every "production" (script->delivery)
@CodeBooster97 I don't know how they run jobs but if they run jobs individually like scripts then they can just reset the value like this:
If they run multiple jobs sequentially at a time then they will have to abstract their job running logic: def do_job(trace_id: str):
audiostack.customer_trace_id = trace_id
# Do the rest of the job...There would be no option to track jobs running in parallel at the moment. I can make it so, but I think it could take up a lot of time to make it clean. |
… tests for trace id handling
| import audiostack | ||
| from audiostack.helpers.request_types import RequestTypes | ||
|
|
||
| _current_trace_id = contextvars.ContextVar("current_trace_id", default=None) |
There was a problem hiding this comment.
I've never seen this before - does this make it safe for using in parallel processes - have you tested?
There was a problem hiding this comment.
Do we guarantee that our SDK is thread-safe?
There was a problem hiding this comment.
According to the python docs ContextVar is threadsafe.
The with(use_trace) block ensures that each thread maintains its own context.
def process_audio(trace_id, text, voice):
with use_trace(trace_id=trace_id):
r = audiostack.Speech.Predict.list()
print(f"Thread {trace_id}: {len(r.data['voices'])} voices available")
r = audiostack.Speech.Predict.predict(text=text, voice=voice)
print(f"Thread {trace_id}: {audiostack.Speech.Predict.interface.make_header()}")
threads = []
for i in range(5):
t = threading.Thread(
target=process_audio, args=(f"trace-{i}", "Hello, world!", "joanna")
)
threads.append(t)
t.start()
for t in threads:
t.join()We can see the threadsafe in action.
The opposite way to make it not threadsafe would be setting the ContextVar as a global.
| any_typed_trace_id: Any = trace_id | ||
| token = _current_trace_id.set(any_typed_trace_id) |
There was a problem hiding this comment.
Have you tried # noqa: F401 to get mypy to ignore the type hints rather than creating an untyped temporary variable
audiostackhenry
left a comment
audiostackhenry
left a comment
There was a problem hiding this comment.
This looks good to me. Just want to to discuss the multithreading bit with you
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def use_trace(trace_id: str) -> Generator[None, None, None]: |
There was a problem hiding this comment.
Should we extend this to add headers too?
There was a problem hiding this comment.
Depends. Do we want to set headers globally in the audiostack object or do we want to set headers per context:
e.g. inside a with() block.
ReferencesI have found a great article that explains exactly what I'm doing. Very nice to follow along: Thread context variables I have also taken a snippet from the docs that also backup what I'm doing. 
Follow up@audiostackhenry No need for 'noqa', just better typing from me @CodeBooster97 have you used contextvars before? A quick look over this PR from you would be useful |
| import audiostack | ||
| from audiostack.helpers.request_types import RequestTypes | ||
|
|
||
| _current_trace_id: ContextVar[Optional[str]] = ContextVar( |
There was a problem hiding this comment.
I've not used ContextVar before.
CodeBooster97
left a comment
CodeBooster97
left a comment
There was a problem hiding this comment.
Read the article about ContextVar. Love the solution, very elegant.
Tested it locally. Can we release this before 13:30 GMT+0 today? I'm having a client call today and they are waiting since 2 weeks for this feature.
from stage_switcher import _get_audiostack_sdk
audiostack = _get_audiostack_sdk()
from audiostack.helpers.request_interface import use_trace
with use_trace(trace_id="Hello World"):
r = audiostack.Speech.Predict.list()
print(len(r.data["voices"]))
r = audiostack.Speech.Predict.predict(
text="shi what are you doiungt", voice="joanna"
)
print(audiostack.Speech.Predict.interface.make_header())
(venv) lars@Larss-MacBook-Pro scripts % python predict.py staging
Headers: {'x-api-key': '', 'x-python-sdk-version': '2.7.1', 'x-customer-trace-id': 'Hello World'}
3 participants
This is a simple change to allow the user to set a trace header so that they can eventually track billing and any other metrics. Will not work with concurrent requests but avoids passing header arguments around. Other potential solutions could involve class level fields, and initiating separate objects.
Also I'm not sure if
customer_trace_idis the best thing to call the customer facing variable. Our header name will stay the same but maybe justtrace_idmight be better, thoughts?I've also added a way to pass through custom headers, this is for when we want to pass through headers set by the FE.