Skip to content

Conversation

@GSVarsha
Copy link
Contributor

@GSVarsha GSVarsha commented Jan 10, 2024

What

Pyramid: capture responseHeadersOnEntrySpans

Signed-off-by: Varsha GS varsha.gs@ibm.com

References

Why

According to our spec, tracers SHOULD capture all four flavours of HTTP headers.

Signed-off-by: Varsha GS <varsha.gs@ibm.com>
Signed-off-by: Varsha GS <varsha.gs@ibm.com>
@GSVarsha GSVarsha self-assigned this Jan 10, 2024
@GSVarsha GSVarsha requested review from Ferenc- and pvital January 10, 2024 09:55
Copy link
Member

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

It appears that the request header capture feature, was previously implemented, but was not covered with UT.
And since this change potentially impacts that feature too, could we add coverage for that too?

Signed-off-by: Varsha GS <varsha.gs@ibm.com>
- test(request_header_capture)

Signed-off-by: Varsha GS <varsha.gs@ibm.com>
@GSVarsha
Copy link
Contributor Author

It appears that the request header capture feature, was previously implemented, but was not covered with UT. And since this change potentially impacts that feature too, could we add coverage for that too?

For sure! Added coverage for capturing request headers in the latest commit.

@GSVarsha GSVarsha requested a review from Ferenc- January 10, 2024 14:13
Copy link
Member

@Ferenc- Ferenc- left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@pvital pvital left a comment

Choose a reason for hiding this comment

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

It looks good to me.

@GSVarsha GSVarsha merged commit 715edda into master Jan 11, 2024
@GSVarsha GSVarsha deleted the pyramid-header-capture branch January 11, 2024 09:13
@GSVarsha GSVarsha added this to the H1-2024 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants