Skip to content

[DRAFT]: feat: use pooled http transport for grafana client #1982

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

eyazici90
Copy link
Contributor

@eyazici90 eyazici90 commented May 4, 2025

Context

Given reusing the same http client for he same host significantly reduces the overhead for dns lookup and handshakes for the same host.

Also leveraging sync.Pool to reduce allocations for geenrating httppool and transportconfig to prevent allocating same structs over again, instead reusing.

This becomes quite important when there are many resources(dashboards, folders, datasources) at thousands level being reconciled concurrently. Reusing the connection for the same host improves a lot.

Benchmark shows an improvement both in speed and mem. usage.

Benchmark_GenGrafanaClient
Benchmark_GenGrafanaClient-12        	  368535	      3266 ns/op	    6130 B/op	      94 allocs/op
Benchmark_GenGrafanaClient_Old
Benchmark_GenGrafanaClient_Old-12    	  307849	      3852 ns/op	    7176 B/op	      97 allocs/op

Related https://github.com/hashicorp/go-cleanhttp/blob/master/cleanhttp.go

@eyazici90 eyazici90 force-pushed the local/pool-client branch 2 times, most recently from 8e4ab9d to 7f4e6a7 Compare May 5, 2025 20:42
@eyazici90 eyazici90 marked this pull request as ready for review May 5, 2025 20:58
@eyazici90 eyazici90 marked this pull request as draft May 5, 2025 21:45
@eyazici90 eyazici90 force-pushed the local/pool-client branch from f0b93c2 to ec3a3c7 Compare May 6, 2025 08:48
@eyazici90 eyazici90 marked this pull request as ready for review May 6, 2025 08:50
eyazici90 added 9 commits May 6, 2025 11:43
Given reusing the same http client for he same host significatnly reduces the overhead for dns lookup and handshakes.

Also leveraging sync.Map to reduce allocations for grafana gen client. This prevents allocating same strcuts, instead reusing.

Signed-off-by: emreya <e.yazici1990@gmail.com>
As it is called from different places.

Signed-off-by: emreya <e.yazici1990@gmail.com>
This is to prevent allocating the struct on the heap. It is being used for comparison in the sync map. Deref is necessary to pass around as value receiver. Benchmark shows value semantic provides great reduce.

Signed-off-by: emreya <e.yazici1990@gmail.com>
Signed-off-by: emreya <e.yazici1990@gmail.com>
For the first call, getAdmin is being called twice. To prevent this, inlining.

Signed-off-by: emreya <e.yazici1990@gmail.com>
Signed-off-by: emreya <e.yazici1990@gmail.com>
…client

This is due to some configs may change during reconcilation, e.g: TLS config. Pooling entire gen-grafana-client may skip the update and leads to unchanged client that requires operator restarts to pick up.

Therefore, pooling for intermediate structs seems more reliable way of reducing alloc.

Signed-off-by: emreya <e.yazici1990@gmail.com>
Signed-off-by: emreya <e.yazici1990@gmail.com>
Signed-off-by: emreya <e.yazici1990@gmail.com>
@eyazici90 eyazici90 force-pushed the local/pool-client branch from eb92df1 to d40d632 Compare May 6, 2025 09:43
@Baarsgaard
Copy link
Collaborator

Baarsgaard commented May 6, 2025

Since you pivoted a bit, would you update the description or leave a comment with some of the considerations?
We squash commits on merge and the detailed commit descriptions are unfortunately lost outside of the PR history.

@eyazici90
Copy link
Contributor Author

Since you pivoted a bit, would you update the description or leave a comment with some of the considerations? We squash commits on merge and the detailed commit descriptions are unfortunately lost outside of the PR history.

Sorry, I didn't understand fully. Do you want me to amend the commit messages 😅 The title remains the same. I updated the PR desc a bit. Lmk, what exactly u want me to update. Happy to follow ;)

@Baarsgaard
Copy link
Collaborator

Baarsgaard commented May 8, 2025

Ah, we do not get notified when the description changes 😅
The Commit messages are fine, I would have liked a some of the considerations in the description or as a comment.
Commit messages are not the easiest thing to find at a glance in the Github UI and is often overlooked.
I will do a review of this quite soon. I'm having to read up on the sync.Pool some more to understand how others use it and what is idiomatic.

Comment on lines +64 to +67
tp := httpTransportPool.Get()
defer func() {
httpTransportPool.Put(tp)
}()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is coming from a limited understanding of sync.Pool, so take it with some salt.
But this defer looks "off".

Every example I see for using sync.Pool always finishes the work before returning the object to the pool.
But here the object is returned to the pool before the client and the containing transports are used.
Could another Goroutine Get() the httpTransport and cfg during a preemption outside of NewGeneratedGrafanaClient and overwrite both?

We have multiple controllers making more than one request during a single reconcile resulting in a lot of preemption opportunities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point. Thanks for noticing that. I reckon, I didn't completely check the control flows and solely focused on my use case. Which was maxConcurrentReconcilation:1 for datasources that was simply serialized execution 🤦

I totally forgot about we can set maxConcurrentReconcilation to more or even different control flows from diff resources here.

I will go for a long weekend vacation and will work on this pooling(sync.Pool) more seriously(not the part for http transport that is pretty much necessary) when I back. Will ping u later on.

Thank u so much for spending ur time on the review, appreciated 🙏

@eyazici90 eyazici90 marked this pull request as draft May 9, 2025 13:48
@eyazici90 eyazici90 changed the title feat: use pooled http transport for grafana client [DRAFT]: feat: use pooled http transport for grafana client May 9, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants