Skip to content

Conversation

@morgan-wowk
Copy link

@morgan-wowk morgan-wowk commented Jan 12, 2026

Issue
#67

Changes:

  • Adds trace context helpers
  • Add trace middleware to apply trace context to API requests
    • This helps logging formatters pull trace ids out of context to set them in logs
    • Sets the trace id on the response for client consumption
  • Wraps API requests with trace context using middleware
  • Passes trace id to pipeline executions using extras parameter
  • Wraps pipeline execution with trace context (using trace id from extras parameter, or creating a new one if None)

Test pipeline notes:

  • Requires ytest, starlette, httpx (e.g. pip3 install ...)

@maxy-shpfy maxy-shpfy requested a review from Ark-kun January 12, 2026 22:55
**Changes:**

* Adds trace context helpers
* Add trace middleware to apply trace context to API requests
    * This helps logging formatters pull trace ids out of context to set them in logs
    * Sets the trace id on the response for client consumption
# Process the request
response = await call_next(request)
# Add trace_id to response headers for client reference
response.headers["x-tangle-trace-id"] = trace_id
Copy link
Author

Choose a reason for hiding this comment

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

Note to self. This line should probably go above await call_next(request) in case an exception is raised

# Restore trace_id from execution
trace_id = None
if queued_execution.extra_data:
trace_id = queued_execution.extra_data.get("trace_id")
Copy link
Contributor

@Ark-kun Ark-kun Jan 13, 2026

Choose a reason for hiding this comment

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

I do not think the request trace IDs are relevant when processing execution nodes. Execution nodes already have their own IDs and they might work better as trace IDs.

@Ark-kun Ark-kun self-assigned this Jan 13, 2026
@Ark-kun
Copy link
Contributor

Ark-kun commented Jan 13, 2026

Thank you for this PR.

I'm reviewing the changes and I think we might want to design this slightly differently.
I think that trace IDs are mostly relevant to API Server and API requests.
Orchestrator works autonomously, it's not triggered by API requests, so it does not have them or need them. Instead, the ExecutionNode.id and ContainerExecution.id already serve the same role.

On the other hand, API requests might benefit from trace IDs so that all logging messages that are generated when processing a single API call can be filtered and grouped together.

@morgan-wowk
Copy link
Author

morgan-wowk commented Jan 13, 2026

Thank you for this PR.

I'm reviewing the changes and I think we might want to design this slightly differently. I think that trace IDs are mostly relevant to API Server and API requests. Orchestrator works autonomously, it's not triggered by API requests, so it does not have them or need them. Instead, the ExecutionNode.id and ContainerExecution.id already serve the same role.

On the other hand, API requests might benefit from trace IDs so that all logging messages that are generated when processing a single API call can be filtered and grouped together.

This sounds good to me.

I will make some adjustments. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants