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

FR: http-request set-priority-class #624

Closed
ShadowJonathan opened this issue Mar 7, 2024 · 19 comments
Closed

FR: http-request set-priority-class #624

ShadowJonathan opened this issue Mar 7, 2024 · 19 comments
Assignees

Comments

@ShadowJonathan
Copy link

ShadowJonathan commented Mar 7, 2024

We have a bunch of different traffic flowing through our application, all of them with different characteristics (latency tolerance, retry resilience, impact on user experience, criticality to proper application functioning, etc.), all of them are identifiable through a variety of different matchings on the request itself.

We have a kubernetes cluster, where currently an nginx ingress controller distributes requests across a variety of api servers. Upstream to that we have another nginx server (frontend) doing the matching, and sending traffic into the kubernetes cluster. We use kubernetes for this, as the volume of traffic ebbs and floods over the day, and auto-scaling on web worker usage and queue length has proven to be an effective method of handling with this.

We want to switch over to using the haproxy ingress controller to take advantage of request prioritisation, as this fits our usecase for managing the above mentioned different traffic patterns. For simplification, the plan was to have the upstream nginx ingress "mark" the requests as such before sending them along, so that a simple haproxy priority class matcher could assign it priority in the queue.

However, it appears that the haproxy ingress controller has not implemented the priority mechanism, as I couldn't find any mention of it in this repository, nor in documentation.

@ivanmatmati
Copy link
Collaborator

Hi @ShadowJonathan , Indeed we didn't implement this possibility. But there's always the possibility to inject your own configuration through config snippets as documented here. If you have already worked out the configuration you can post it there so we can discuss its integration. If you don't know how to do it, please ask the Haproxy Slack channel.

@ShadowJonathan
Copy link
Author

I haven't yet spotted the config snippet option, when I do get it working i'll add the snippet here for anyone else to use, thank you for mentioning it! 💚

@ShadowJonathan
Copy link
Author

Here's the config snippet:

    haproxy.org/backend-config-snippet: |
      acl traffic_ap_g req.hdr(x-traffic-class) -m str AP_G
      acl traffic_ap_p req.hdr(x-traffic-class) -m str AP_P
      acl traffic_ua_g req.hdr(x-traffic-class) -m str UA_G
      acl traffic_ua_p req.hdr(x-traffic-class) -m str UA_P
      acl traffic_anon req.hdr(x-traffic-class) -m str ANON
      acl traffic_admi req.hdr(x-traffic-class) -m str ADMI

      http-request set-priority-class int(2) if traffic_ap_g
      http-request set-priority-class int(1) if traffic_ap_p
      http-request set-priority-class int(0) if traffic_anon
      http-request set-priority-class int(-1) if traffic_ua_g
      http-request set-priority-class int(-2) if traffic_ua_p

      http-request set-priority-class int(-10) if traffic_admi

The header is set upstream at the aforementioned nginx proxy.

Here's an example of some graphs of how this stacks the traffic patterns;
image
image

It fits our usecase perfectly. :)

Copy link

stale bot commented Apr 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 10, 2024
@ShadowJonathan
Copy link
Author

Oh nice, another repo to add to the list: https://nostalebots.xyz

@stale stale bot removed the stale label Apr 10, 2024
Copy link

stale bot commented May 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 10, 2024
@ShadowJonathan
Copy link
Author

Thanks for reminding me to add it to the list.

@stale stale bot removed the stale label May 10, 2024
Copy link

stale bot commented Jun 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 10, 2024
@ShadowJonathan
Copy link
Author

No.

@stale stale bot removed the stale label Jun 10, 2024
Copy link

stale bot commented Jul 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 10, 2024
@ShadowJonathan
Copy link
Author

No.

@stale stale bot removed the stale label Jul 11, 2024
@ShadowJonathan
Copy link
Author

@ivanmatmati why did you close this?

@ivanmatmati
Copy link
Collaborator

Why should it be kept open ?

@ShadowJonathan
Copy link
Author

It hasn't been formally closed, the stale bot does not substitute a reason for why it should be closed; Are you not interested in adding this feature? If not, thats a reasonable reason to close, but inactivity is not.

@ivanmatmati ivanmatmati self-assigned this Jul 11, 2024
@ivanmatmati
Copy link
Collaborator

Inactivity is usually a good reason to close a stale issue. In this case, it's not. Considering the config snippet you provided, it seems that it's highly personal and not reusable for a general case. It's unclear for me if you found the desired implementation in NGINX. If so can you point to the documentation ?

@ivanmatmati ivanmatmati reopened this Jul 11, 2024
@ShadowJonathan
Copy link
Author

I used a combination of mapped variables and setting headers to be passed to the proxy upstream to map the priorities. The config snippet works as-is, but requires knowing the set-priority-class option(s), so possibly providing an easier way to expose this kind of usecase to the user would make sense.

I understand not wanting to develop that though, and so this issue can be closed on that basis, but I object more to the idea of issues being closed based on inactivity, with the idea that the feature's need has not waned in the meantime. Workarounds would decrease traffic to the issue, since there is an idea that its being worked on, and github heavily disincentivises "bumping" issues, since it just adds noise, so issue threads are worked out until the issue itself is made clear, and then its left alone to be picked up like work items. Stale bots fundamentally misunderstand this idea, and close nicely-worked-out issues, simply because bumping it does not happen.

@oktalz
Copy link
Member

oktalz commented Jul 15, 2024

hi @ShadowJonathan,
I've looked at the link you provided (https://nostalebots.xyz/)

it does have some good marks but also some bad ones. In the end it all comes to this:

Priority is very subjective, and is also something that the maintainers need to determine about a particular issue, weighing tons of different variables together

And I agree on this, we as a maintainers decided stale bot is good for us.
Not every company/project has the same system and that is fine. In fact not all projects have same settings.
Also, its not just about priority, there is a lot of things in projects that we all do differently, priority is just one factor.
Having stalebot can be seen as good and bad (or both at the same time too). at the time we added it, we had a lot of answered questions that were left opened and that added to the noise since we do go over them.

It hasn't been formally closed, the stale bot does not substitute a reason for why it should be closed

It does say it, issue was opened, had some questions and answer, there were no activity or something new that would contribute to the topic added . Could the answer be better, maybe, but the fact is, system we put in place works like that. I can admit that we do not have a really large setup for stalebot, but it does mostly what we think it should do.
Number of issues is not a problem, if you check the repo you will see some pretty old issues that are still open, so bot is not closing everything.

This issue is actually a good example why keeping this open adds more unnecessary work for maintainers in future, not to mention potential confusion.

For example:

  1. I need to comment adding of that link (nostalebots)
  2. now we have new option to configure this, with backend CRD. That eliminates the need for config snippet solution.

Going through all open issues and update them, while they can be closed (because other parts of project like documentation provide solutions) is more work, also we can discuss whether commenting/updating on old issues is good or bad. We prefer to have more clean documentation where options are more structured.
K8s world is heavily customized, almost every user has a specific setup, should we support all of them ?
In ideal world maybe, but we would either ended up with some monster that is simply to complex, or a lot of time would be spent to combine everything.

As a good example here, we had few PRs to improve our TCP handling, we had some suggestions, conversations and in the end we had a different solution that IMHO solves problems users had.

fact that this is also the third comment that has nothing to do with the issue itself (first being nostalebots ). It just adds noise to the issue (If that is a problem a new issue could be opened with that subject)

we are open to new ideas, also there has been some community PRs too, some are open due to rules we have, some needs to be looked at, some i closed today because we had alternatives (and that was commented)

Oh nice, another repo to add to the list: https://nostalebots.xyz/

is it nice? would it be better to leave this issue with main topic and open maybe another one with this subject instead of just passively saying how this is nice because we disagree how project is currently handling this.
Over time things change, managing project too. There is always room to improve things (for example, even code base with 100% test coverage can produce panics)

Workarounds would decrease traffic to the issue, since there is an idea that its being worked on, and github heavily disincentivises "bumping" issues, since it just adds noise

Github is good and bad at the same time, some features are great, some are not, a lot of things can be improved and same can be told with this project too.

we do not have project label to mark this issue with to much noise, and I feel that putting invalid here is not correct since we did had a good conversation to a point, so I'll just close the issue (it can still be commented).

@oktalz oktalz closed this as not planned Won't fix, can't repro, duplicate, stale Jul 15, 2024
@benaryorg
Copy link

This issue is actually a good example why keeping this open adds more unnecessary work for maintainers in future, not to mention potential confusion.

I see about 8 unnecessary interactions between user and stalebot here, something that does generate notifications and noise for maintainers, and then after that I see interactions with about 700 words written on the topic of stalebots and barely anything related to the issue at hand.

I think you will agree that:

  • handling this situation here has been significantly more time consuming than a single response from a maintainer that could've been a clear and concise "this can be used via a custom config snippet, currently there are no plans to add this feature" closing the issue with no further interaction needed on the maintainer side
  • Apiv1beta1 #608 is a ridiculously good example of why closing something may cause actual work done by actual contributors is more likely to get lost forever (besides that PR just feels incredibly rude like what the hell? Are you sure you want to be like this to contributors?)
  • closing an issue never makes the actual problem go away. Feel free to tag something as stale, but don't just close it.

To quote the same text that you've described as "does have some good marks but also some bad ones":

What's more, issues aren't processed, they are discarded, they are not regarded and/or triaged in most cases, which is a way your project can become unreliable and broken, as you blind your developers to potential or ongoing issues.

When I look through the closed stale issues I see on the first page a single issue that was also tagged as "waiting-for-feedback". I won't link the issue because that would be unnecessary noise. That one issue however is a prime example of how the process should work; a maintainer sees that feedback is needed, tags the issue, and then after some time the stalebot could close the issue. That's still not great, but it's better. Better than closing an issue where other people are waiting on a reply from the maintainers. On the matter of waiting on a reply from a maintainer, I see you do have a tag called "investigation" (which the project settings describe as "more investigation needed on our side"). I also see on the same first page of closed stale issues one issue which was closed by the stalebot, which also looks very much like the problem wasn't solved, and there still was something to do for the maintainers since it has been tagged as "investigation", but instead some noise was created for everyone involved and the issue was removed from sight, despite there supposedly being something to fix.


I see no gain for the project by closing any of it automatically, but I do see a hostility towards users and contributors when you say something like:

Going through all open issues and update them, while they can be closed (because other parts of project like documentation provide solutions) is more work

You're specifically saying that you have read an issue (otherwise you wouldn't know whether or not it can be closed), but that the time it would take you to answer "there is an option for this in the documentation, please have a look" and tag it as "needs feedback" is worth more than the time a user has to read and respond to a stalebot in a field that is well known to have a lot of people with social anxiety and generally introverts working in it so that both of us know very well that the time they spend on writing a reply to a stalebot is not just going to be the time it takes to write "this is still current", but is probably much more in the range of tens of minutes. Your one minute for typing what I suggested at the beginning of this paragraph is worth more than their potential half hour? That would be a vile thing to say.

As a good example here, we had few PRs to improve our TCP handling, we had some suggestions, conversations and in the end we had a different solution that IMHO solves problems users had.

And by your account the stalebot was a good thing in that context? So you're saying that not providing that context in the issue, not providing the information that a consensus was reached about there being a better way to handle the technical details is the good thing to do? To just.… leave the issue open without any distinct resolution? The next person is going to look at an issue that was closed by the stalebot and is gonna ask "but.… what am I supposed to do now?". At the same time the action of actually closing the issue and tagging it as "won't fix" or something takes you a fraction of the time that the alleged conversations have taken, so why omit that one step? Why rely on a stalebot that does the same work you do but significantly worse and makes the entire situation really hard to follow for anyone coming after?

K8s world is heavily customized, almost every user has a specific setup, should we support all of them ?

Of course you are free to steer the project any direction you like, but if you value your users, you are going to tell them what direction this is, instead of just ignoring them and waiting for their issue to close automatically just to irritate and confuse everybody.
Instead of telling the user to provide more information in half the issues you simply ignore them with the implicit expectation of "elaborate on your issue", but with no way for the user to discern that intent. Imagine you're the user instead, sitting there with your open issue, waiting for someone to respond (unknowing that they have already read your issue and just didn't respond because they wait for the stalebot to close the issue, for whatever reason (I cannot help but suspect an intention of "I don't want to be blamed for closing this, then I'd be the bad guy, let stalebot be the bad guy instead")) so you just sit there until the stale period runs out, time that you may have repeatedly checked on the issue, hoping for an answer, and then you are greeted by the issue just being closed out of nowhere when you thought you were getting a reply. Is this satisfying? Would you care to report another issue to that project?
Sure, it may decrease the amount of noise you get from having to actually manually close an issue (or tag it appropriately) but at the same time the pool of users who will report issues will always tend towards the people who are not very good at triage and bug reporting, because everyone who puts effort into writing a proper report will leave after their first stalebot encounter. You're actively discouraging the people who know how to write a bug report from investing time in your project, so your issue quality will only ever decrease over time. Stalebots configured to close issues are a glorified footgun and we all know it, but what's worse, you're making everyone around you angry for firing that gun repeatedly and making everyone else spend time cleaning up after your gunshots (issues that shouldn't have been closed).

Ignoring the other party of a conversation and instead waiting for them to get bored staring at your back and departing is just not a thing that a decent human being would do in real life, so why is it okay to do the same by means of a stalebot on an issue tracker, when the alternative is to spend one minute of your time replying to the issue that allegedly you've already read anyway?

@ivanmatmati
Copy link
Collaborator

ivanmatmati commented Jul 16, 2024

@ShadowJonathan, I've just come across the new contents of your "website" about stale bots where you put a picture of one of my interaction with a user in this github repository.

This excerpt causes problems. First, it misleads people to think that after having said that I'll get back to the user, I haven't and finally the stale bot has to enter in action.
As you are prolific in advices, I take it you will accept this one: do a fair and better job when you want to criticize. If you looked carefully at the interaction with this user, you would have noticed that the PR you mentioned was superseded by an other one from him on the same thing and the very next day I mentioned I'll get back to him. In case you need some guidance:
#608
#609

As you can see I told him I'll get back to him on 30th Jan, and he opened the next one on 31th Jan ...
This poor example from you even contradicts any assumption made about our work. We don't let users down. Actually, we and other people are waiting for his follow-up since 3rd April ...

Second, on these dedicated Github pages we invite people to discuss technical issues, ask technical questions or technical feature requests. The two keywords are "invite" and "technical". The fact that we invite people in this space means you must behave correctly and respect its purpose. So any sarcastic and irrelevant comments about stale bots like "Oh nice, another repo to add to the list: https://nostalebots.xyz" or "Thanks for reminding me to add it to the list" are not correct.

Third, as my personal name appears in your "website" in a biased excerpt that suggests something plainly wrong, I strongly advise that you remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants