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

add request hook for http client request spans #529

Merged
merged 2 commits into from
Apr 11, 2019
Merged

Conversation

rochdev
Copy link
Member

@rochdev rochdev commented Apr 8, 2019

This PR adds a request hook for http client request spans. This is needed to be able to update the resource name since otherwise there is no way to get a reference to the span.

This is a breaking change since hooks that were registered for servers in the shared http config will start being executed for client requests as well.

Closes #524

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

Any way to reasonable structure them so they aren't configured via the same hook?

@rochdev
Copy link
Member Author

rochdev commented Apr 9, 2019

@brettlangdon It can be done using the client key. However, for historical reason setting any configuration directly on the root will configure both the server and client.

@brettlangdon
Copy link
Member

Ok, so it isn't unreasonable or unexpected that client and server would be configured via the same root/shared configuration. The expected part is for anyone who is already using these hooks that they will start to receive client requests as well when they weren't before?

@rochdev
Copy link
Member Author

rochdev commented Apr 9, 2019

I think it can always be unexpected, because this was basically a design flaw when we only supported clients in the http integration. When we added support for the server they shared the same config. I split them with the server and client keys but kept the shared config for backward compatibility.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

got it.

@rochdev rochdev merged commit 7bf8073 into v0.11.0 Apr 11, 2019
@rochdev rochdev deleted the http-client-hooks branch April 11, 2019 12:56
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.

2 participants