Skip to content

Conversation

@ericcarlschwartz
Copy link
Contributor

@jpeach
Copy link
Contributor

jpeach commented Apr 22, 2016

Please fix the commit message to refer to TS-4379. The commit message should also contain some more information, take a look this.

The logic here needs to be aligned with #554, please sync up with @jacksontj and @bgaff.

Has this been benchmarked? ConnectionCount::getCount() effectively takes a global lock, so I would expect some performance impact.

Are logs really the right way to expose this information? Maybe it would be better to be able to extract more general statistics about how Traffic Server is utilizing origins?

@jacksontj
Copy link
Contributor

IMO logging the connection count isn't the right way to go-- as the connections (number of them and lifecycle) are independent of the actual transactions that go across them (separate timers, min_origin_connections, etc.). As far as logging is concerned we already have a field for number of requests on the connection-- which is the best we can do (as thats the only overlap between the two).

I'm not sure how we want to expose this metric, but I'm thinking we either add it to the regular stats interface (my concern being duplicating the metric), or make something like the web interfaces for connection counts (http API to get the table dumped directly), thoughts?

@zwoop
Copy link
Contributor

zwoop commented Apr 25, 2016

I think I agree with Thomas. This sets a bit of a bad precedence, treating one metric as special over all other metrics by also giving it a log tag. I'd much prefer something more generic, at least long term, such that all metrics could also be logged using a generic tag. E.g.

%<{proxy.process.http.total_server_connections}met>

@SolidWallOfCode
Copy link
Member

Yeah. We could look at something similar to the milestone work, a generic tag that lets you pull out a metric.

@bryancall
Copy link
Contributor

@zwoop This is a value per origin, not a single number that can be represented in the metrics/stats.

I agree, having something that can generically log stats would be helpful, but it couldn't be used in this case.

@ericcarlschwartz ericcarlschwartz changed the title [YTSATS-676] Add log field for Server Connection Count [TS-4379] Add log field for Server Connection Count Apr 25, 2016
@zwoop
Copy link
Contributor

zwoop commented Apr 25, 2016

Gotcha. Maybe that should be a regular metric instead then? Per-origin metric? Although, that has to be configurable to turn off, because it could explode in your face (e.g. if you run as a forward proxy).

@bryancall
Copy link
Contributor

It is configurable to turn it off and on.

@zwoop
Copy link
Contributor

zwoop commented Apr 25, 2016

Right, I meant, maybe it should be an option to enable these per origin metrics in the core. At which point, the generic "metric" log tag would work, right?

But, I don't want to stand in the way of progress here. I'm ok with landing this, but we should file appropriate Jira's such that we can do these metrics and log tags generically. And then deprecate these features here.

@jacksontj
Copy link
Contributor

jacksontj commented Apr 25, 2016

@zwoop @bryancall My main point here is that putting this in the access logs isn't the right place to put it as the transactions have little to do with the connections. I also want this metric, but the only way to be even close to correct is to do one of the 2 options I mentioned above (ATS metric, or http UI interface thing).

Otherwise this will just tell you what the number was at the time we logged it-- which (IMO) isn't all that helpful-- since presumably you'd want to graph how close you are to per-origin limits etc-- which the logs wouldn't do.

@JamesPeach
Copy link

@bryancall AFAICT server_connection_count is always populated, which is hitting a global lock. If there's no consensus that this is the right approach then I would prefer to not merge this.

@shinrich
Copy link
Member

@jacksontj the way we are using it on the log entries is as a boolean (zero or non-zero) to understand whether that request was able to reuse an existing origin connection. We are doing log analysis at various levels (requesting clients, domains, etc) and merging up attributes.

@jpeach I think I had done some performance analysis on this a while back, and it didn't seem to have a significant impact. I'll repeat that analysis and provide more concrete impact #'s.

@JamesPeach
Copy link

A log field to mark whether a server session was re-used seems helpful, but a very different patch from what is being proposed here.

@bryancall
Copy link
Contributor

@jacksontj Having it part of the existing metrics would mean you would have a metric per origin. We went down that road before and created a stats v2 which was a mess.

A better design might be to have framework for having metrics in the server connection pool and have a management API to query this information. Also, it can be logged like @zwoop mentioned above.

The number of requests on the connection could be another metric that would be interesting.

@jacksontj
Copy link
Contributor

Right, I think the correct way was my option #2-- which would be to have an API for getting this info (basically another mgmt API endpoint like hostdb, stats, etc.).

@zwoop
Copy link
Contributor

zwoop commented Apr 25, 2016

I agree with @jacksontj , having an API here makes sense. Then you defer any penalty cost to the actual usage of such APIs as well (so no cost when not used). The downside being that we currently have no way to include that in the log formats.

@zwoop zwoop added the Logging label May 12, 2016
shinrich pushed a commit to shinrich/trafficserver that referenced this pull request Jan 9, 2018
Prevent releasing streams simultaneously
ywkaras pushed a commit to ywkaras/trafficserver that referenced this pull request Jul 7, 2022
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request Sep 26, 2023
* Check ResourceLocalManager startup before inactivity cop

* fix unit tests

* remove flag, use state to indicate startup complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants