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

move metrics to a common interface/implementation #108

Closed
wants to merge 1 commit into from

Conversation

srivastavankit
Copy link
Contributor

allow reuse for http request / client module.

@@ -0,0 +1,58 @@
package zanzibar
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a copyright header here

@Raynos
Copy link
Contributor

Raynos commented May 23, 2017

Honestly, I don't like this version of the code. I'd rather not add a lock when the existing code does not have a lock.

@srivastavankit
Copy link
Contributor Author

@Raynos The existing version of the code pre-allocates all possible error codes as a metric. For an HTTP Client, this means we end up creating this map for each method on the client. Seems super expensive to do this much pre-allocation and premature optimization as well.

If we need to support lazy initialization of the map, we need to use mutex make sure multiple threads dont write to the map.

@Raynos
Copy link
Contributor

Raynos commented May 24, 2017

@srivastavankit

~800 counters takes up 70KB.
Adding another ~75,000 counters takes up 40MB.

Using more memory to avoid per-request locks seems reasonable to me. That being said we should benchmark the performance difference and look at flamegraphs.

@Raynos
Copy link
Contributor

Raynos commented May 24, 2017

Looking at benchmarks and flamegraphs

The lock does not impact performance at all but the extra sprintf causes a regression from 1.3% in Flush() -> 2% in Flush().

Since it's per-request critical path I would favor the more performant version.

@Raynos
Copy link
Contributor

Raynos commented Oct 2, 2017

PR is stale. Will close for now. Feel free to re-open if still wanted.

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.

None yet

3 participants