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.

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

Remove non-combined route generation #3131

Closed
4 tasks
rainest opened this issue Nov 4, 2022 · 5 comments · Fixed by #4749
Closed
4 tasks

Remove non-combined route generation #3131

rainest opened this issue Nov 4, 2022 · 5 comments · Fixed by #4749
Assignees
Labels
breaking change release/required it is required that this be resolved before releasing
Milestone

Comments

@rainest
Copy link
Contributor

rainest commented Nov 4, 2022

Following the addition of HTTPRoute combined route generation support, traditional route generation is deprecated and will be removed in a future release. Combined route generation results in significant reductions in configuration size in many configurations and improves config apply time, while not affecting request routing. There are no known benefits to traditional route generation over combined route generation, and only a minor transition hurdle due to changing route names and IDs.

For users

Please comment on this issue if you observe any issues with combined route generation or gaps in functionality between it and traditional route generation.

For developers

To remove traditional route generation, we need to:

  • Remove the feature gate from feature gate handling code. Mark a breaking change in the changelog for users to remove it from their feature gate envvar (unrecognized gates block startup).
  • Mark the feature graduated in FEATURE_GATES.md.
  • Remove traditional generation code from the parser/translator and remove any switches/ifs that choose between it and combined generation.
  • Remove any tests dedicated to traditional and update any tests that did not account for combined routes.
@rainest rainest added breaking change release/required it is required that this be resolved before releasing labels Nov 4, 2022
@rainest rainest added this to the KIC v3.0.0 milestone Nov 4, 2022
@robinmanuelthiel
Copy link

For me, the combined routes don't support wildcards: #3311

@Clint-Chester
Copy link

Clint-Chester commented Jan 15, 2023

Unfortunately we recently upgraded our KIC from v2 to v2.8 and didn't realise that the route generation was now rolled up. We were using the non-combined route generation so that we could do performance monitoring on the individual routes with the Kong Grafana Dashboard. The upgrade resulted in breaking our monitoring capability by route.

Since #3132 was feature flagged, could this just be exposed as an annotation to give users choice on whether they'd like the routes rolled up (like it can be here #3311 (comment))? Or is there another way that we can monitor the routes in a service? Happy to go into more detail with the use case if you like 😄

@rainest
Copy link
Contributor Author

rainest commented Jan 17, 2023

More detail would help, yes. You should still be getting per-route monitoring information, it's just that the route names have changed, correct? The rollup doesn't remove routes per se, it consolidates them, so while there will be a break in analytics data it should be a one-off thing.

@Clint-Chester
Copy link

Yeah in our dashboards it rolled up all our routes into one, so we lost our analytics of individual paths. I'll go through the example we were using.

So we have the ingress defined below as an example:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: sample-ingress
spec:
  rules:
  - host: "test.com"
    http:
      paths:
      - pathType: Prefix
        path: "/validate/email"
        backend:
          service:
            name: backend-service
      - pathType: Prefix
        path: "/validate/mobile"
        backend:
          service:
            name: backend-service

Even though they are going to the same backend / host / port, we want to specify the individual paths so that we can get statistics on the individual route in terms of latency and upstream processing performance. I thought this was a nice little feature to be able to start breaking down individual routes for performance monitoring, without being intrusive on adjusting an application's architecture.

Unfortunately in the above example, this would all get rolled into one route and specify multiple paths within that route.

The ideal target state that we were looking for in the implementation is the ability to get metrics on different paths hitting the same backend for performance analysis and localising where errors are coming from (which we could before the change).

Happy to provide more info if needed 😄

@rainest
Copy link
Contributor Author

rainest commented Jan 25, 2023

That's a necessary tradeoff we have to make to reduce configuration size (and experience has shown we very much want to--larger configurations are demonstrated to slow configuration updates down considerably, to the point that they start failing altogether). The reduction in route count necessarily implies that some metrics data is aggregated for all paths in the route.

Keeping the paths separate in metrics with the aggregated/combined would require separating them into different Ingresses, or making a feature request for https://docs.konghq.com/hub/kong-inc/prometheus/ that adds separate buckets with a path component for kong_request_latency_ms_bucket{service="google",route="google.route-1",le="700"} 1 and similar metrics.

Alternatively, are you collecting metrics via any other methods? The logging plugins do include path information (see https://docs.konghq.com/hub/kong-inc/udp-log/#log-format for an example--all the logging plugins use the same JSON format). Those wouldn't provide time series data, but the per-request data may be a viable option depending on what types of analysis you need to perform and what other tools you use--Kibana or similar should be able to run ad-hoc queries that aggregate the individual values into buckets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change release/required it is required that this be resolved before releasing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants