Reuse connections in postgresql metricsets #12603
Conversation
jsoriano
added
module
review
Metricbeat
jsoriano
force-pushed
the
postgresql-keep-connections
branch
from
8e1b3e1
to
801f9ad
Compare
jsoriano
force-pushed
the
postgresql-keep-connections
branch
from
801f9ad
to
e68e916
Compare
|
|
||
| results, err := postgresql.QueryStats(db, "SELECT * FROM pg_stat_statements") | ||
| ctx := context.Background() | ||
| results, err := m.QueryStats(ctx, "SELECT * FROM pg_stat_statements") |
There was a problem hiding this comment.
I'm guessing metricset Close won't cancel these requests, I guess it would be unexpected for them to leak, but it could be worth it to handle ctx cancellation?
There was a problem hiding this comment.
Right, Close doesn't cancel running queries, they will be executed till they finish. I wouldn't expect leaks because of that.
Not sure if it would worth to hande cancelation here, we'd need to keep a thread safe list of cancelation functions available on close. Once we have #11981 or similar we will be able to cancel the parent context when calling the closers.
There was a problem hiding this comment.
++ I thought about that PR too. That said, you don't really need to keep a list, isn't it? When creating the metricset you can create a Background context, use it in all fetch operations and cancel it on close.
There was a problem hiding this comment.
Having that that PR is moving (:crossed_fingers:) I'm ok with leaving this for update after it's merged
There was a problem hiding this comment.
That said, you don't really need to keep a list, isn't it? When creating the metricset you can create a Background context, use it in all fetch operations and cancel it on close.
Yes, this would be another option, but I think this is what should be done in a general what by #11981. Doing it this way here has some disadvantages, we'd be hiding the context inside the metricset and it would be a different one to the one received by Fetch if #11981 is implemented.
Having that that PR is moving (crossed_fingers) I'm ok with leaving this for update after it's merged
Yeah, let me move this forward 🙂
exekias
added
the
needs_backport
jsoriano
removed
the
needs_backport
Reuse connections in PostgreSQL metricsets instead of opening and
closing one on each fetch.
Fixes #12504