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.

Sign up for GitHub

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

Jump to bottom

Log davs:// https:// transfer stats in the logs #1536

Closed
wants to merge 1 commit into from
Closed

Log davs:// https:// transfer stats in the logs #1536

wants to merge 1 commit into from

Conversation

juztas
Copy link
Contributor

@juztas juztas commented Oct 13, 2021

This is what it will give in the logs (based on the http.trace flag stats|statsall:

211013 16:26:47 2630835 http_Req: ALL_STATS_REPORT: retval=501, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=POST, reqstate=0, remote=[2605:d9c0:1:9::1:1]; Request not supported.
211013 16:26:47 2631149 http_Req: ALL_STATS_REPORT: retval=207, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=PROPFIND, reqstate=0, remote=[2605:d9c0:1:9::1:1];
211013 16:26:47 2630834 http_Req: ALL_STATS_REPORT: retval=501, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=POST, reqstate=0, remote=[2605:d9c0:1:9::1:1]; Request not supported.
211013 16:26:47 2630835 http_Req: ALL_STATS_REPORT: retval=207, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=PROPFIND, reqstate=0, remote=[2605:d9c0:1:9::1:1];
211013 16:26:47 2631149 http_Req: ALL_STATS_REPORT: retval=200, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=GET, reqstate=0, remote=[2605:d9c0:1:9::1:1]; STAT
211013 16:26:47 2631149 http_Req: STATS_REPORT: retval=200, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=GET, reqstate=1, remote=[2605:d9c0:1:9::1:1]; OPEN
211013 16:26:47 2631149 http_Req: ALL_STATS_REPORT: retval=200, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=GET, reqstate=1, remote=[2605:d9c0:1:9::1:1]; FINISHED
211013 16:26:55 2631149 http_Req: STATS_REPORT: retval=200, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=GET, reqstate=2032, remote=[2605:d9c0:1:9::1:1]; OPEN-FINISH
211013 16:26:55 2631149 http_Req: STATS_REPORT: retval=200, filename=/store/user/jbalcas/2/3/5.root, user=jbalcas, reqtype=GET, reqstate=2032, remote=[2605:d9c0:1:9::1:1]; FINISH-DONE-OK

There might be need to change/add more TRACE - but this can be done later.

@bbockelm please review. Thanks!

bbockelm
bbockelm suggested changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@bbockelm bbockelm left a comment

Choose a reason for hiding this comment

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

Hi Justas!

Looks great! Thanks for the contribution! A few minor items, mostly on styling. I'm happy with the approach, of course, because it's quite similar to what is done in XrdTpc.

Brian

src/XrdHttp/XrdHttpReq.cc Outdated
prot->rec.paramsSet = true;
prot->rec.filename = resource.c_str();
prot->rec.remote = prot->SecEntity.host;
prot->rec.name = prot->SecEntity.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

SecEntity.name is potentially a nullptr. If it's an unauthenticated client (plain HTTP), this will segfault.

src/XrdHttp/XrdHttpReq.cc Outdated
prot->SendSimpleResp(200, NULL, NULL, (char *) mydata->data, mydata->len, keepalive);
reset();
return keepalive ? 1 : -1;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid non-essential whitespace movement in a commit with functional changes.... can you undo these and squash?

src/XrdHttp/XrdHttpProtocol.cc Outdated
@@ -184,6 +184,7 @@ SecEntity(""), CurrentReq(this) {
Addr_str = 0;
Reset();
ishttps = imhttps;
rec = LogRecord();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the reset of rec be in the Reset() function? That function is called whenever the XrdHttpProtocol object is reused for future, unrelated requests.

src/XrdHttp/XrdHttpReq.cc Outdated
//
if (!prot->rec.paramsSet) {
prot->rec.paramsSet = true;
prot->rec.filename = resource.c_str();
Copy link
Contributor

Choose a reason for hiding this comment

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

What c_str() here instead off the copy constructor?

src/XrdHttp/XrdHttpReq.cc
// Do a Stat
if (prot->doStat((char *) resourceplusopaque.c_str())) {
XrdOucString errmsg = "Error stating";
errmsg += resource.c_str();
logTransferEvent(TRACE_STATSALL, 404,(char *) errmsg.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the const-cast here? That suggests maybe the definition of logTransferEvent is wrong?

src/XrdHttp/XrdHttpReq.cc Outdated
prot->rec.status = retval;
std::stringstream ss;
ss << "retval="<< prot->rec.status << ", filename=" << prot->rec.filename;
ss << ", user=" << prot->rec.name << ", reqtype=" << prot->rec.reqtype;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we skip the user= record when rec.name is empty?

src/XrdHttp/XrdHttpReq.cc Outdated

void XrdHttpReq::logTransferEvent(const int debug, const int retval, const char *message)
{
if (retval != -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we short circuit the creation of the std::stringstream if all debugging is disabled?.

@juztas juztas force-pushed the logdavhttpstransferstats branch 3 times, most recently from 0abff3f to e4ab758 Compare October 14, 2021 23:27
@abh3
Copy link
Member

abh3 commented Oct 26, 2021

Before I merge this I will wait some comments about the issue I raised in pull request 1529 (i.e. using a monitoring stream for this level of detail) from you and Brian.

@juztas
Copy link
Contributor Author

juztas commented Dec 8, 2021

@bbockelm Can you comment on this and can we get this finally merged, so it reaches 5.4 xrootd release?

@abh3
Copy link
Member

abh3 commented Dec 16, 2022

OK, so what we actually did, in keeping with xroot philosophy, is to report TPC details in the G-stream. The philosophy is that statistics are only to be reported in a monitoring stream. The log file is not to be used for such purposes (no matter how popular that seems to be in K8s). Hence, I am closing this pull request.

@abh3 abh3 closed this Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbockelm bbockelm bbockelm requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@juztas @abh3 @bbockelm