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

varnishd: add backend age & reuses to Open & Close #4007

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

Conversation

asadsa92
Copy link
Contributor

This extra information comes handy when trubleshooting idle timeout from the backend.

include/tbl/vsl_tags.h Outdated Show resolved Hide resolved
include/tbl/vsl_tags.h Outdated Show resolved Hide resolved
bin/varnishd/cache/cache_conn_pool.c Outdated Show resolved Hide resolved
@asadsa92 asadsa92 force-pushed the backend_reuse branch 2 times, most recently from 227df30 to bb5da78 Compare August 19, 2024 12:22
@asadsa92
Copy link
Contributor Author

Sorry for the long RTT, I must have forgotten about it.
All of your comments should have been addressed now, if not let me know.

@varnishcache varnishcache deleted a comment from gquintard Aug 19, 2024
Copy link
Member

@nigoroll nigoroll left a comment

Choose a reason for hiding this comment

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

OK, as I have given up arguing for use of VTIM_mono...

@asadsa92 asadsa92 requested a review from dridi November 6, 2024 16:04
Comment on lines 67 to 69
vtim_real created;
uint64_t reused;
Copy link
Member

Choose a reason for hiding this comment

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

I think @nigoroll has a point (although he didn't try to make it) that in this case we don't need real time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @nigoroll , I intended to ack on it but forgot.
I have made the switch over to vtim_mono.

Comment on lines 143 to 144
"\t| | | | | | | | +- Connection uses\n"
"\t| | | | | | | +------ Connection age\n"
Copy link
Member

Choose a reason for hiding this comment

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

Apologies for flip-flopping on this one but looking at the reused field it should probably say "Connection reuses" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed.

Comment on lines 132 to 133
vtim_real
PFD_Age(const struct pfd *p, vtim_real now)
Copy link
Member

Choose a reason for hiding this comment

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

This function should return a vtim_dur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 332 to 335
VSLb(bo->vsl, SLT_BackendOpen,
"%d %s %s %s %s %s reuse %.6f %ju", *fdp,
VRT_BACKEND_string(dir), abuf2, pbuf2, abuf1, pbuf1,
PFD_Age(pfd, VTIM_real()), PFD_Reused(pfd));
Copy link
Member

Choose a reason for hiding this comment

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

There's no point having a now argument in PFD_Age() if we end up passing VTIM_real(), we just create ceremony. My expectation was that we should have a ctx->now, or one ctx->wrk->lastused freshly computed from the last timestamp update.

If we are not reusing a real timestamp available in this scope then there is no point having a now argument or using real time at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have for now dropped the now argument. In the future we might add it back as vtim_mono, but let's not delve into this right now.

This extra information comes handy when trubleshooting idle timeout from the
backend.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
"\t| | +------------- Remote address\n"
"\t| +---------------- Backend display name\n"
"\t+------------------- Connection file descriptor\n"
"\t%d %s %s %s %s %s %s [ %.6f %ld ]\n"
Copy link
Member

Choose a reason for hiding this comment

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

I just realized we don't specify the precision in the manual, we simply go for %f (probably from a log consumer point of view).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point. Did not realize we have not done this in the past.
I am leaning toward consistency, but having the precision allows you to select the correct data type to store this value.
Are we fine making such promises? or should I simply use %f here?

Copy link
Member

Choose a reason for hiding this comment

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

We should put %f here to be consistent with the rest of the tags.

@@ -45,6 +45,8 @@ struct pfd;

unsigned PFD_State(const struct pfd *);
int *PFD_Fd(struct pfd *);
vtim_real PFD_Age(const struct pfd *);
Copy link
Member

Choose a reason for hiding this comment

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

vtim_dur

Comment on lines 142 to 146
CHECK_OBJ_NOTNULL(p, PFD_MAGIC);
now = VTIM_mono();
assert(now >= p->created);

return (now - p->created);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need an assertion here ¯\(ツ)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken form man 2 clock_gettime:

 All CLOCK_MONOTONIC
              variants guarantee that the time returned by  consecutive  calls
              will not go backwards, but successive calls may—depending on the
              architecture—return identical (not-increased) time values.

Copy link
Member

Choose a reason for hiding this comment

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

In general, if you switched to vtim_mono now (thank you), I would prefer to not call variables holding it now, but rather t_mono or somthing.

Copy link
Contributor Author

@asadsa92 asadsa92 Nov 8, 2024

Choose a reason for hiding this comment

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

I did actually grep before naming the variable:

$ git grep -rne "now = VTIM_mono"
bin/varnishd/mgt/mgt_vcl.c:676:         now = VTIM_mono();
bin/varnishd/mgt/mgt_vcl.c:1089:        now = VTIM_mono();
bin/varnishstat/varnishstat_curses.c:1181:              now = VTIM_mono();
lib/libvarnishapi/vsl_dispatch.c:942:   now = VTIM_mono();
lib/libvarnishapi/vsl_dispatch.c:1453:  now = VTIM_mono();
lib/libvarnishapi/vut.c:401:    now = VTIM_mono();

I can rename the one in this PR here, or we can handle it as one commit tree-wide after this PR has gone in.

Copy link
Member

@nigoroll nigoroll Nov 8, 2024

Choose a reason for hiding this comment

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

side note: we got no type safety of the three vim_* types. Years ago I attempted struct-wrapping them, which achieved the goal, but was messy. So for now, we need to be careful to not confuse them.

Signed-off-by: Asad Sajjad Ahmed <asadsa@varnish-software.com>
@asadsa92
Copy link
Contributor Author

asadsa92 commented Nov 8, 2024

Thanks for the review comments, it should have been addressed now.
Let me know if this needs more bikeshedding.

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.

3 participants