Hi Oren, Sorry for the slow response, I think what you are proposing sounds reasonable, so +1. I would, however, like to make sure that we aren't affecting the performance of TM too much in terms of both CPU usage (with 1000+ caches) and time to poll all caches/aggregate statistics.
Additionally, I don't think we should be adding new functionality to astats at all. The plan is to move towards the Stats Over HTTP plugin in the future, so we should focus on making Traffic Monitor do what we need it to do now, and add new functionality to Stats over HTTP if it doesn't currently exist. I do think it would be nice to have Traffic Monitor support some sort of canonical stats interface instead of astats/stats_over_http, and I am sure there are several other features would be nice to have as well, but I don't think we should be muddying the waters with that right now. On Wed, Dec 27, 2017 at 12:24 PM, Robert Butts <[email protected]> wrote: > I'd be +1 if we extend Astats/stats_over_http to have the DS name. > > It's easy to extend the Monitor to get the name from a field, if it's > available. And it should be faster than the current method. But, Astats > doesn't currently support it, and in fact, I don't believe ATS has a good > way to associate an identifier (DS name) with a Remap Rule. Can anyone > confirm? > > That said, if we're extending `stats_over_http`, I'd really like to see it > extended to serve CSV (ideally with the DS name in a field, as above), when > the request has an `Accept: text/csv` header. Astats only has a single > level of JSON, and they're all numbers. So there's no reason to use JSON > over CSV, and CSV is drastically simpler and faster to parse. Especially if > we break each `.` component of the stat into its own CSV field. The Monitor > is CPU-bound, so there's a good chance we'd see a noticeable performance > improvement, which translates to a tangible poll time improvement. > > Also @orens if we can't reasonably extend ATS Astats, we can still support > your use case for your own cache, by having a separate code path in the > Monitor for your "stats", and adding a "stats type" number or enum to the > Server table (or ideally a new table for "cache servers", since it only > applies to caches). So, the Monitor would look at that field (possibly > adding it to `monitoring.json` or `CRConfig.json`), to determine how to > parse a given server's stats. > > We've discussed a "stat version" field anyway, for different Astats > versions. For example, we're looking into combining the System stats into > the Ats key, and we'd also like to be able to experiment with different > formats (CSV, Protobuf, etc). I think it's a good feature for the CDN in > general, to make stat polling more flexible. > > > On Wed, Dec 27, 2017 at 12:21 PM, Durfey, Ryan <[email protected]> > wrote: > > > Oren, > > > > This is a great conversation to have. I can’t speak to the best solution, > > but allowing our servers to have a single SSL cert that could be shared > > across delivery services would save us a lot of time, money, and > > operational resources. From a product perspective this would be highly > > beneficial to the platform especially since all services are eventually > > moving to SSL. > > > > Key Benefits: > > > > 1. Instant Service Builds: All new services could use SSL immediately, > > no 2-3 business day delays in ordering SSL certs. > > * This gets us a step closer to instant SSL service builds without > > the need to implement something like “letsencrypt”. > > 2. Operations Load: Reducing the management of a constant flow of > > orders and renewals for SSL certs would save operations resources and > time. > > 3. Cost Reduction: A reduction in SSL certs equates to reduction in > > purchase and renewal costs. > > > > > > > > Ryan Durfey M | 303-524-5099 > > CDN Support (24x7): 866-405-2993 or [email protected]<mailto: > > [email protected]> > > > > > > From: Oren Shemesh <[email protected]> > > Reply-To: "[email protected]" < > > [email protected]> > > Date: Wednesday, December 27, 2017 at 2:49 AM > > To: "[email protected]" < > > [email protected]> > > Subject: Two different Delivery Services using the same host regexp: A > > suggested Traffic Monitor code change > > > > Hi TC Dev people, > > > > We would like to avoid having to issue a specific SSL certificate for > every > > DS, as is needed today. > > > > (Reminder: Every DS needs a separate certificate because The domains used > > by the DS are tr.<ds-name>.<cdn-domain> and > > <cache-name>.<ds-name>.<cdn-domain>, and wild-card certs support only a > > single *, so every cert is issued to *.<ds-name>.<cdn-comain>, which > makes > > it DS-specific). > > > > When configuring two different DSes to use the same host regexp, and > > configuring additional path regexps to differentiate between them, it is > > possible to overcome this issue. > > > > For example: DS 'my-ds1' can have host regexp .*\.my-ds\..* , and path > > regexp /ds1/.* (Both with order 0), and DS 'my-ds2' has the same host > > regexp and path regexp /ds2/.*. > > > > This makes TR redirect requests of the form > > tr.my-ds.<cdn-domain>/ds1/<whatever> to > > <cache-name>.my-ds.<cdn-domain>/ds1/<whatever>, do the same for > > tr.my-ds.<cdn-domain>/ds2/<whatever>, and respond with '503 service > > unavailable' to requests to tr.my-ds.<cdn-domain> which do not start with > > /ds1/ or /ds2/. > > > > So with TR, So far so good. > > > > ATS can also differentiate between the DSes based on the path, so there > is > > no problem there as well. > > > > However, there is a problem with Traffic Monitor: > > > > Because astats sends per-DS info based on the origin name, TM requires > that > > every DS has a unique host regexp (See > > https://github.com/apache/incubator-trafficcontrol/blob/ > > 3e3a5f41d482a065f46ef287b347e66d7d205d82/traffic_monitor/ > > todata/todata.go#L234 > > ). > > This is so that TM can map origin names from astats back to a DS. > > > > So, I would like to make a small change to the TM code: > > > > If the astats info contains only a single host name, assume it is already > > the DS name. > > > > (This would be implemented in <goog_1101383350> > > https://github.com/apache/incubator-trafficcontrol/blob/ > > 3e3a5f41d482a065f46ef287b347e66d7d205d82/traffic_monitor/ > > cache/cache.go#L419 > > : > > > > if len(statParts) == 2 { // [0] is DS name, [1] is statName > > ds = statParts[0] > > } > > else { > > ... existing code which uses statsParts[0:len(statsParts)-1] > > } > > > > Obviously, the ds should be checked against the list of all DSes to make > > sure it is valid, I wrote this code line just to demonstrate the idea). > > > > Of course, the astats plugin for ATS might not support this today (I am > not > > sure), but a different cache (Hint: Qwilt cache) has no problem reporting > > the astats information per DS name. > > If needed, the astats code could be improved to do that, as well. > > > > I believe that this change, while being very small and unobtrusive, would > > make the information passed from caches to TM 'cache vendor agnostic', > > while also eliminating the need to re-classify URLs into DS (Which is > what > > happens today in TM), hence it is a good step in a good direction. > > > > Any comments / opinions, please ? > > > > -- > > > > *Oren Shemesh* > > Qwilt | Work: +972-72-2221637| Mobile: +972-50-2281168 | [email protected] > > <mailto:[email protected]> > > <[email protected]<mailto:[email protected]>> > > > > >
