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

Make use of pre-existing context in InstrumentRoundTripperTrace #582

Merged
merged 2 commits into from
May 16, 2019

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented May 16, 2019

Fixes #580

@shturec does this do the trick?

@shturec
Copy link

shturec commented May 16, 2019

Yep, seems ok to me.
I didn't see you have made a PR already and shot my own (#584) and I've contributed two tests inside that used to verify the behavior with context (using timeout context for the experiment). They work also with your solution so I think it's fine.
Feel free to make use of them if you find them ok.

@beorn7
Copy link
Member Author

beorn7 commented May 16, 2019

Thanks. Having tests for this is great. However, we don't want to pull in a dependency just for assertion. If you don't mind, I will just incorporate the essence of your tests into this PR.

@beorn7 beorn7 force-pushed the beorn7/promhttp branch 2 times, most recently from dee343d to 56d3658 Compare May 16, 2019 15:51
Tests are heavily inspired by @shturec, see #584.

Signed-off-by: beorn7 <bjoern@rabenste.in>
Fixes #580

Signed-off-by: beorn7 <bjoern@rabenste.in>
@beorn7
Copy link
Member Author

beorn7 commented May 16, 2019

I think I got it all nicely packaged up.

@beorn7
Copy link
Member Author

beorn7 commented May 16, 2019

Incidentally, this also fixes the 1st flaky test mentioned in #573

@beorn7 beorn7 merged commit ed5d97d into master May 16, 2019
@beorn7 beorn7 deleted the beorn7/promhttp branch May 16, 2019 16:03
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.

Request context is overridden in instrumented http clients
2 participants