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

feat: Add trace ID to the response header. #24

Merged
merged 6 commits into from
Sep 20, 2023

Conversation

lafer-fz
Copy link
Contributor

What type of PR is this?

feat: Add trace ID to the response header.

What this PR does / why we need it (English/Chinese):

During the debugging process, it may be necessary to use the trace ID to troubleshoot issues. As a server-side developer, I would appreciate it if others could provide me with the trace ID of the problematic requests.

Which issue(s) this PR fixes:

Fixes(#23)

@rogerogers
Copy link
Collaborator

Maybe we should add a switch and an option for the header key configure.

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2023

CLA assistant check
All committers have signed the CLA.

@lafer-fz
Copy link
Contributor Author

Yes, I think what you said is very accurate. I have made the changes, please review again.

@rogerogers
Copy link
Collaborator

The configuration of HTTP header names may be very useful, as some one may want to use x-trace-id, x-request-id, and so on.

@lafer-fz
Copy link
Contributor Author

Absolutely, configuring the HTTP header names would indeed be valuable, as different individuals might prefer using various names like x-trace-id, x-request-id, and so on. While HTTP header names should be configurable, I'd like to suggest having a default value: x-b3-traceid. The reason behind choosing x-b3-traceid as the default is because the Heartz library employs the B3 implementation of propagation.TextMapPropagator by default, which utilizes x-b3-traceid as the HTTP header name for trace IDs.
image
image

@rogerogers
Copy link
Collaborator

I think the default trace-id is sufficient. "b3" is a header used for zipkin distributed tracing. When used in the response header, it can be a bit confusing.

@lafer-fz
Copy link
Contributor Author

Okay, use trace-id by default.

@CoderPoet
Copy link
Collaborator

@lafer-fz Thank you very much for your contribution, this is a very useful feature.

However, the usual practice in the community is to abstract a mechanism for custom response headers, which is indeed more generic.

Consider referring to the Javaagent version implementation: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/bootstrap/http/HttpServerResponseCustomizer.java

The traceid response header can be considered as a way to become a plugin integrated by default

/cc @rogerogers

@rogerogers
Copy link
Collaborator

I've been quite busy lately, i'll take a look at the Java implementation later. Additionally, Java itself is a bit more flexible.

@li-jin-gou
Copy link
Contributor

PATL @rogerogers @CoderPoet

Copy link
Collaborator

@CoderPoet CoderPoet left a comment

Choose a reason for hiding this comment

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

/LGTM

@CoderPoet CoderPoet merged commit 7f92845 into hertz-contrib:main Sep 20, 2023
10 checks passed
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.

None yet

5 participants