Skip to content

do not expose the trace_id in response header right now#2061

Merged
bnjjj merged 4 commits intodevfrom
bnjjj/rollback_response_header
Nov 8, 2022
Merged

do not expose the trace_id in response header right now#2061
bnjjj merged 4 commits intodevfrom
bnjjj/rollback_response_header

Conversation

@bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Nov 8, 2022

No description provided.

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj self-assigned this Nov 8, 2022
@github-actions

This comment has been minimized.

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

In the same spirit, should we hold off on returning it in the traces to Apollo Studio since it that won't be an accurate representation of what headers were actually returned?

let mut response_headers = HashMap::with_capacity(1);
response_headers.insert(
String::from("apollo_trace_id"),
Values {
value: vec![span.span_context.trace_id().to_string()],
},
);

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj
Copy link
Contributor Author

bnjjj commented Nov 8, 2022

@abernix I commented the part related to studio too

@bnjjj bnjjj requested a review from abernix November 8, 2022 12:05
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Copy link
Contributor

@o0Ignition0o o0Ignition0o 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 the ci issue is because we need to bump the version (the fuzzer is relying on a yanked crate)

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj enabled auto-merge (squash) November 8, 2022 17:16
@bnjjj bnjjj merged commit ad20599 into dev Nov 8, 2022
@bnjjj bnjjj deleted the bnjjj/rollback_response_header branch November 8, 2022 17:33
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.

4 participants