Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate trace context to webhook and upload requests #1698

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

aron
Copy link
Contributor

@aron aron commented May 29, 2024

If the request to /predict contains headers traceparent and tracestate defined by w3c Trace Context1 then these headers are forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

Footnotes

  1. https://www.w3.org/TR/trace-context/

@aron aron requested a review from nickstenning May 29, 2024 10:15
@aron aron force-pushed the propagate-trace-headers branch from e123a02 to 8b61718 Compare May 29, 2024 10:15
Copy link
Contributor

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

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

I think this is ok, but this seems like a clear-cut case where we could use a ContextVar and a couple of helper functions (in, say, a telemetry module) to make this a lot less messy.

The idea would be something like:

# telemetry.py
from contextlib import contextmanager
from contextvars import ContextVar

TRACE_CONTEXT = ContextVar("trace_context", default=None)

def current_trace_context():
    return TRACE_CONTEXT.get()

@contextmanager
def trace_context(ctx):
    t = TRACE_CONTEXT.set(ctx)
    try:
        yield
    finally:
        TRACE_CONTEXT.reset(t)

# http.py
async def predict(
    request: PredictionRequest = Body(default=None),
    prefer: Union[str, None] = Header(default=None),
    traceparent: Union[str, None] = Header(default=None),
    tracestate: Union[str, None] = Header(default=None),
) -> Any:  # type: ignore
    ...
    ctx = {}
    if traceparent:
        ctx['traceparent'] = traceparent
    if tracestate:
        ctx['tracestate'] = tracestate

    if ctx:
        with telemetry.trace_context(ctx):
            return predict(...)
    
    return predict(...)

@aron
Copy link
Contributor Author

aron commented May 29, 2024

Okay, nice, I'll refactor the code.

Copy link
Contributor

@nickstenning nickstenning left a comment

Choose a reason for hiding this comment

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

Generally LGTM other than the handling of absent/None values. The branch diff here also introduces a bunch of spurious whitespace and formatting changes which it would maybe be nice to rebase out, but that's not a huge deal.

@aron
Copy link
Contributor Author

aron commented May 29, 2024

The branch diff here also introduces a bunch of spurious whitespace and formatting changes which it would maybe be nice to rebase out, but that's not a huge deal.

Hmm, I used ruff format and ruff check to format and validate the files respectively. Perhaps we're not enforcing consistent formatting?

@aron aron force-pushed the propagate-trace-headers branch from 9cbdb24 to 625137b Compare May 29, 2024 14:39
@aron
Copy link
Contributor Author

aron commented May 29, 2024

Hmm, @nickstenning the ContextVar doesn't seem to propagate across the threads, does this need to be handled manually?

Nope, I'm a dope and wasn't adding the headers to the upload client.

@aron aron force-pushed the propagate-trace-headers branch 4 times, most recently from 8156993 to e2e0673 Compare May 29, 2024 15:27
aron added 2 commits June 3, 2024 12:28
If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[1]: https://www.w3.org/TR/trace-context/

Signed-off-by: Aron Carroll <[email protected]>
@aron aron force-pushed the propagate-trace-headers branch from e2e0673 to 0fd5314 Compare June 3, 2024 11:34
@aron
Copy link
Contributor Author

aron commented Jun 3, 2024

I've now rebased on main and run the python directory through ruff:

 python -m ruff check python --fix

I'm leaving that as the canonical whitespace.

@aron aron merged commit 817f34c into main Jun 3, 2024
15 checks passed
@aron aron deleted the propagate-trace-headers branch June 3, 2024 13:47
aron added a commit that referenced this pull request Jul 4, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/
aron added a commit that referenced this pull request Jul 4, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/
aron added a commit that referenced this pull request Jul 4, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/
aron added a commit that referenced this pull request Jul 11, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/
aron added a commit that referenced this pull request Jul 11, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/
technillogue pushed a commit that referenced this pull request Jul 12, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 12, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 18, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 18, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
mattt pushed a commit that referenced this pull request Jul 18, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
mattt pushed a commit that referenced this pull request Jul 19, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 24, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 25, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 25, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
technillogue pushed a commit that referenced this pull request Jul 26, 2024
Based on the implementation in #1698 for sync cog.

If the request to /predict contains headers `traceparent` and
`tracestate` defined by w3c Trace Context[^1] then these headers are
forwarded on to the webhook and upload calls.

This allows observability systems to link requests passing through cog.

[^1]: https://www.w3.org/TR/trace-context/

Signed-off-by: technillogue <[email protected]>
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