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

feat: avoid rewrapping response writer when capturing metrics #22

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

costela
Copy link
Contributor

@costela costela commented Jul 20, 2022

This is a first attempt at addressing #19.

First we refactor the common code for rw from being generated to sitting in its own file.

Then we add an indirection to Wrap, so that it becomes possible to associate the metrics with the returned http.ResponseWriter.

Then we add an interface to recover the Metrics from a wrapped ResponseWriter. If we can do that, then we directly call the wrapped function and return the existing Metrics.

This unfortunately introduces an asymmetry: consumers of the library that use Wrap() directly will not be able to use Metrics(). That kinda makes sense: Wrap() is not necessary used for metrics. We could move the metrics init to Wrap() and always track them, but that would probably be an unexpected semantic change (with probable performance impact of its own). So I opted against it.

WDYT? Does the general direction make sense?

Closes #19

@costela costela marked this pull request as draft July 20, 2022 16:28
@costela costela marked this pull request as ready for review July 21, 2022 07:32
@costela costela force-pushed the avoid-duplicate-wrappings branch from f98f37a to d4ff2da Compare July 21, 2022 07:58
@costela costela changed the title feat: avoid rewrapping response writer feat: avoid rewrapping response writer when capturing metrics Jul 21, 2022
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.

avoid multiple wrapping
1 participant