Proposal: proxy-less mode

[vbehar]

This is a proposal for a new feature, so we can get an agreement before starting coding ;-)

The idea is to allow multiple implementations of the "metrics collector", with the default one still being the osiris auto-injected proxy, and with at least a new one: a prometheus-based metrics collector.
This new collector would collect metrics from an already existing prometheus endpoint exposed by the pod. It would need the following input:

• port exposed by the pod on which the prometheus endpoint is available
• path on which the prometheus metrics data is exposed. default to /metrics
• metrics names to collect. To be compliant with the current (and default) metrics collector, we would need 2 metrics: 1 for the opened connections, and another one for the closed connections.

This new feature will bring the following benefits:

• complete control over how a request is counted, ie no need to use the ignoredPaths, and if needed requests can be ignored based on different input: user-agent, source IP, ...
• allow the use of another tool that inject a transparent proxy as a sidecar container, like a service mesh.
• avoid the "cost" of the proxy, and the possible issues that could come from using it (Failed requests with Osiris proxy #45 for example...)

I was thinking about adding a new annotation on the deployments: osiris.deislabs.io/metricsCollector, using a JSON value - similar to what datadog is doing with https://docs.datadoghq.com/getting_started/integrations/prometheus/?tab=kubernetes :

metadata:
  annotations:
    osiris.deislabs.io/metricsCollector: |
      {
        "type": "prometheus",
        "implementation": {
          "port": "8080",
          "path": "/metrics",
          "metrics": {
            "openedConnections": "http_req_new",
            "closedConnections": "http_req_closed"
          }
        }
      }

this JSON would have the following schema:

• type: name of the collector implementation. default to osiris
• implementation: a RawJSON that each implementation can use as they see fit.
  we could also imagine moving the osiris.deislabs.io/metricsCheckInterval annotation to a checkInterval field here - to avoid too many annotations.

what do you think ?

[krancour]

In one of the earlier prototypes of Osiris, Prometheus was used for collecting metrics.

iirc, we ripped it out because asking users (especially ones who might already have been using it) to configure Prometheus a particular way undermined the "it just works" narrative.

I can see what you have proposed as being, perhaps, more tenable. If you don't want to mess with your Prometheus configuration or don't want to run Prometheus at all, Osiris still works out-of-the-box with its default metrics collector. "Advanced users" can have the option to do things differently.

That being said, I'm a little bit reluctant to accept this level of new complexity, even as an option, primarily because I can't guarantee any significant resource commitments to this project at the moment.

I can say that I'd at least be willing to entertain some kind of PoC here, but the likelihood of moving forward or not is going to end up being directly proportionate to how much complexity and/or maintenance burden the PoC shows needs to be added.

[vbehar]

Hi, thanks for the answer. ok, I'll write a quick POC to see how it goes.

[josephpage]

I'm a big fan of the idea of an advanced configuration for power users/use cases.

Although I prefer a multi-annotations configuration over one big JSON :

osiris.deislabs.io/inject-proxy: "false" # default "true", when true the other annotations are useless
zeroscaler.osiris.deislabs.io/metrics-collector: prometheus  # default "prometheus"
zeroscaler.osiris.deislabs.io/metrics-port: "9001" # default "9090"
zeroscaler.osiris.deislabs.io/opened-connections-metric: http_req_new # default "http_req_new"
zeroscaler.osiris.deislabs.io/closed-connections-metric: http_req_closed  # default "http_req_closed"

I have taken inspiration from what Nginx Ingress Controller does.

[aledbf]

Although I prefer a multi-annotations configuration over one big JSON :

@josephpage please don't follow any of those approaches. The only reason why ingress-nginx uses/d annotations is that at the time (four years ago) there was no other option. This kind of customization should be done with CRDs

[krancour]

This kind of customization should be done with CRDs

That won't get merged. Osiris has deliberately shunned the use of CRDs and embraced annotations to maximize simplicity. The idea was that you use normal resources the same as you would if scale-to/from-zero were not a concern and then you enable that feature by annotating those resources. I'm relatively neutral on whether that was a good decision or not, but my point is that we're not going to reverse course on that at this stage-- especially given that (as I've noted before) there isn't a large resource commitment to this project at the moment.

So, all of the above being said:

Although I prefer a multi-annotations configuration over one big JSON

☝️ I agree with this.

[aledbf]

That won't get merged.

@krancour to be clear, I am not trying to propose this change, just talking about my (bad) experience with annotations, once you have more than just a few (mainly due to the lack of real types, where everything is a string and interactions between multiple annotations)