Copilot commented on code in PR #13344:
URL: https://github.com/apache/trafficserver/pull/13344#discussion_r3488660778
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -810,6 +811,7 @@ HttpTunnel::add_consumer(VConnection *vc, VConnection
*producer, HttpConsumerHan
// Register the consumer with the producer
p->consumer_list.push(c);
p->num_consumers++;
+ ATS_PROBE5(tunnel_add_consumer, sm->sm_id, static_cast<int>(vc_type),
static_cast<int>(p->vc_type), p->num_consumers, skip_bytes);
Review Comment:
This ATS_PROBE call is a single long line, which makes the surrounding
changes harder to read/maintain. Wrap the arguments across lines (matching the
style used for the other probe calls in this file).
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1289,6 +1293,8 @@ HttpTunnel::producer_handler(int event,
HttpTunnelProducer *p)
bool sm_callback = false;
Dbg(dbg_ctl_http_tunnel, "[%" PRId64 "] producer_handler [%s %s]",
sm->sm_id, p->name, HttpDebugNames::get_event_name(event));
+ ATS_PROBE6(tunnel_producer_handler, sm->sm_id, event,
static_cast<int>(p->vc_type), p->read_vio ? p->read_vio->ndone : 0,
+ p->bytes_consumed, p->ntodo);
Review Comment:
This probe call line is still overly long and will be difficult to scan in
traces/reviews. Consider wrapping after the vc_type argument to keep within the
project's typical formatting/line-length conventions.
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -499,6 +502,8 @@ CacheVC::openReadReadDone(int event, Event *e)
}
if (writer_lock_retry < cache_config_read_while_writer_max_retries) {
DDbg(dbg_ctl_cache_read_agg, "%p: key: %X ReadRead retrying: %"
PRId64, this, first_key.slice32(1), vio.ndone);
+ ATS_PROBE6(cache_rww_reader_starve, stripe->fd,
reinterpret_cast<int64_t>(this), first_key.slice64(0), vio.ndone, doc_len,
+ writer_lock_retry);
Review Comment:
Casting pointers to int64_t is implementation-defined and can truncate or
sign-mangle on unusual platforms/ABIs. Prefer intptr_t for pointer-to-integer
trace payloads.
##########
src/iocore/cache/CacheRead.cc:
##########
@@ -292,6 +293,8 @@ CacheVC::openReadFromWriter(int event, Event *e)
vector.insert(&alternate);
alternate.object_key_get(&key);
write_vc->f.readers = 1;
+ ATS_PROBE6(cache_rww_reader_attach, stripe->fd, first_key.slice64(0),
reinterpret_cast<int64_t>(this),
+ reinterpret_cast<int64_t>(write_vc), cod->num_writers,
write_vc->total_len);
Review Comment:
Casting pointers to int64_t is implementation-defined and can truncate or
sign-mangle on unusual platforms/ABIs. Prefer intptr_t for pointer-to-integer
trace payloads.
##########
src/proxy/http/HttpTunnel.cc:
##########
@@ -1510,6 +1518,8 @@ HttpTunnel::consumer_handler(int event,
HttpTunnelConsumer *c)
HttpTunnelProducer *p = c->producer;
Dbg(dbg_ctl_http_tunnel, "[%" PRId64 "] consumer_handler [%s %s]",
sm->sm_id, c->name, HttpDebugNames::get_event_name(event));
+ ATS_PROBE5(tunnel_consumer_handler, sm->sm_id, event,
static_cast<int>(c->vc_type), c->write_vio ? c->write_vio->ndone : 0,
+ c->write_vio ? c->write_vio->nbytes : 0);
Review Comment:
This probe call is still quite long; wrapping it more aggressively improves
readability and reduces the chance of future formatting churn.
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -864,6 +866,7 @@ SSLNetVConnection::load_buffer_and_write(int64_t towrite,
MIOBufferAccessor &buf
} break;
}
}
+ ATS_PROBE5(net_ssl_write, this->get_fd(), total_written, write.vio.ndone,
num_really_written, static_cast<int>(err));
Review Comment:
The probe currently reports write.vio.ndone before the caller accounts for
total_written. Using ndone + total_written makes the trace payload consistent
with other write probes that report the updated cumulative byte count.
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -960,6 +962,7 @@ Http2ConnectionState::rcv_window_update_frame(const
Http2Frame &frame)
if (error != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, error,
"Erroneous client window update");
}
+ ATS_PROBE4(http2_window_update_rcvd, this->session->get_connection_id(),
0, size, this->get_peer_rwnd());
Review Comment:
Use the named constant for the connection-control stream ID instead of the
literal 0 to avoid a magic number and make the probe payload self-describing.
##########
src/iocore/cache/CacheWrite.cc:
##########
@@ -585,6 +588,8 @@ CacheVC::openWriteMain(int /* event ATS_UNUSED */, Event *
/* e ATS_UNUSED */)
vio.get_reader()->consume(avail);
vio.ndone += avail;
total_len += avail;
+ ATS_PROBE6(cache_rww_writer_produce, stripe->fd,
reinterpret_cast<int64_t>(this), first_key.slice64(0), total_len, avail,
+ od ? od->num_writers : 0);
Review Comment:
Casting pointers to int64_t is implementation-defined and can truncate or
sign-mangle on unusual platforms/ABIs. Prefer intptr_t for pointer-to-integer
trace payloads.
##########
src/iocore/cache/CacheWrite.cc:
##########
@@ -277,6 +278,8 @@ CacheVC::openWriteCloseDir(int /* event ATS_UNUSED */,
Event * /* e ATS_UNUSED *
ink_assert(!is_io_in_progress());
VC_SCHED_LOCK_RETRY();
}
+ ATS_PROBE6(cache_rww_writer_close, stripe->fd,
reinterpret_cast<int64_t>(this), first_key.slice64(0), closed,
+ static_cast<int>(f.readers), total_len);
Review Comment:
Casting pointers to int64_t is implementation-defined and can truncate or
sign-mangle on unusual platforms/ABIs. Prefer intptr_t for pointer-to-integer
trace payloads.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
