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

add utility function for retrieving a trace id #1663

Merged
merged 10 commits into from
Sep 2, 2022
Merged

Conversation

garypen
Copy link
Contributor

@garypen garypen commented Aug 31, 2022

add utility function for retrieving a trace id

Retrieve a unique trace id which can be used to link together processing
steps in a plugin (or anywhere else in the router).

If an execution trace context cannot be established, the return value
will be None To correctly confirm that two things are linked you
would have to check:

let other_id = apollo_router::tracer::maybe_trace();
let my_id = apollo_router::tracer::maybe_trace();

// Note: enough to check that at least one of your traces is some...
if my_id.is_some() && my_id == other_id {
    <blah...>
}

Without that first check, i.e.:

if my_id == other_id {
    <blah...>
}

you could be linking together two unrelated things.

fixes: #1536

Retrieve a unique trace id which can be used to link together processing
steps in a plugin (or anywhere else in the router).

If an execution trace context cannot be established, the return value
will be INVALID_TRACE (all zeros). Watch out for this. Two invalid
traces will be equal to each other. In other words, to correctly confirm
that two things are linked you would have to check:

let other_id = apollo_router::trace_id();
let my_id = apollo_router::trace_id();

if my_id != INVALID_TRACE && my_id == other_id {
    <blah...>
}

Without that first check, i.e.:

if my_id == other_id {
    <blah...>
}

you could be linking together two unrelated things which both had
invalid ids.

fixes: #1536
@garypen garypen self-assigned this Aug 31, 2022
to changelog
or circleci isn't happy.
Add a to_u128() fn and modify Display to use this.

Also, replace to_bytes() with as_bytes().
@Geal
Copy link
Contributor

Geal commented Sep 1, 2022

If an execution trace context cannot be established, the return value
will be TraceId::INVALID (all zeros)

why not return an Option?

@@ -69,6 +69,7 @@ pub mod services;
mod spec;
mod state_machine;
mod test_harness;
pub mod tracer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer tracing as I could see us adding other trace related things in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sigh... If I rename to tracing it creates several hundred ambiguity errors with the tracing crate. Probably best to leave as tracer.

Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

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

Approved, but see comment on module name.

@garypen
Copy link
Contributor Author

garypen commented Sep 1, 2022

If an execution trace context cannot be established, the return value
will be TraceId::INVALID (all zeros)

why not return an Option?

I had that first, but decided to mirror the behaviour in the existing interface (which does this). I do think the nice thing about the Option is that we'd unlock all the combinators and it would mean we could cut out the check to make sure it's not INVALID.

I'll have a look at re-shaping this with an Option and see if it looks better.

If we get TraceId::INVALID, then return None from the constructor.
to match implementation
By adding a mutex to the tests to serialize them with respect to global
resources.
to make it better
@garypen garypen merged commit 13042ad into main Sep 2, 2022
@garypen garypen deleted the garypen/1536-trace-id branch September 2, 2022 09:55
@abernix abernix mentioned this pull request Sep 2, 2022
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.

linking requests and responses
4 participants