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

Update http attrs #272

Closed
wants to merge 9 commits into from
Closed

Update http attrs #272

wants to merge 9 commits into from

Conversation

meiao
Copy link
Contributor

@meiao meiao commented Apr 9, 2021

Overview

Adds http status code/text to external spans and web transaction spans.

Related Github Issue

#225

Checks

Are your contributions backwards compatible with relevant frameworks and APIs?
Not entirely.

Does your code contain any breaking changes? Please describe.
Yes.
There is a change in the signature of one the HttpParameters constructors. The constructor is protected, so it would only affect people who subclassed HttpParameters. This can be avoided by creating a new constructor.
A method was added to the Build interface of the HttpParameters$Builder. This will only affect people that implemented that interface.

Does your code introduce any new dependencies? Please describe.
No.

@@ -63,6 +63,7 @@ private static void processResponse(URI requestURI, HttpResponse response) {
.uri(requestURI)
.procedure(PROCEDURE)
.inboundHeaders(inboundCatWrapper)
.status(response.getStatusLine().getStatusCode(), response.getStatusLine().getReasonPhrase())
Copy link
Contributor Author

@meiao meiao Apr 9, 2021

Choose a reason for hiding this comment

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

This is what every instrumentation will have to add.
The new method was added to the last interface of the step builder (Build).
So existing code can continue to call the build method, or add a call to the new status method and then call build.

* Set the status code/text of the HTTP response.
* @return the builder in a buildable state.
*/
Build status(Integer statusCode, String statusText);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is another breaking change. But should only cause problems to someone implementing this interface.
Not sure why one would do it.

@meiao meiao marked this pull request as ready for review May 4, 2021 13:56
@meiao meiao closed this May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant