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.

Sign up for GitHub

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

Jump to bottom

Pyramid: capture responseHeadersOnEntrySpans #496

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Pyramid: capture responseHeadersOnEntrySpans #496

merged 4 commits into from
Jan 11, 2024

Conversation

GSVarsha
Copy link
Contributor

@GSVarsha GSVarsha commented Jan 10, 2024 •
edited
Loading

What

Pyramid: capture responseHeadersOnEntrySpans

Signed-off-by: Varsha GS [email protected]

References

Why

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

@GSVarsha GSVarsha self-assigned this Jan 10, 2024
@GSVarsha GSVarsha requested review from pvital and Ferenc- January 10, 2024 09:55
Ferenc-
Ferenc- requested changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@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?

@GSVarsha
minor fixes
bac4026 
Signed-off-by: Varsha GS <[email protected]>
@GSVarsha
- fix(request_header_capture)
7e03907 
- test(request_header_capture)

Signed-off-by: Varsha GS <[email protected]>
@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
Ferenc-
Ferenc- approved these changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@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

pvital
pvital approved these changes Jan 11, 2024
View reviewed changes
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
2 checks passed
@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
Reviewers

@pvital pvital pvital approved these changes

@Ferenc- Ferenc- Ferenc- approved these changes

Assignees

@GSVarsha GSVarsha

Labels
enhancement
Projects
None yet
Milestone
H1-2024
Development

Successfully merging this pull request may close these issues.
3 participants
@GSVarsha @pvital @Ferenc-