-
Notifications
You must be signed in to change notification settings - Fork 101
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
Improve ztunnel dashboard UI #638
Comments
Doesn't logging in envoy use a POST as well? Modifications should require a POST? Or are we just talking about viewing the current level? For stats, we don't need to copy envoy IMO. |
We are talking about viewing the current level, currently it's not viewable directly like envoy.
+1, I think |
I can do the pretty print and log level |
I don't think this is completely true. I can get the logging level of ztunnel through an empty post:
Though, it is much less descriptive and formatted differently than envoy:
Neither ztunnel nor enovy support GET requests for So I wouldn't say that " /logging is unusable since it expects a POST and a level," but the ztunnel api is definitely different than the envoy API. |
@Stevenjin8 Thanks for testing this. We are talking about the
I'm not sure, but I think we may need to modify the dashboard page, and perhaps the API a little, to make it work? |
@hanxiaop Thank you for that. I was able to reproduce. From a brief look at the HTML, the difference lies in the dashboard page rather than the API. The issue with the current ztunnel dashboard page is that it uses a hyperlink to reference the <a href="logging">logging</a> This means that when you click on the endpoint, the browser will send a GET request to the The envoy logging dashboard uses buttons instead of hyperlinks which allows it to send post requests. This is kinda hacky thought because if you go to the logging endpoint and then refresh, the page, you'll get an error. It also looks kinda janky because there are a mix of buttons and hyperlinks I think the better solution is to have envoy and ztunnel accept GET requests. If we just want to mirror behavior, we can replace the hyperlink with a button. |
@Stevenjin8 I think adding a GET request to Envoy may not be acceptable since POST has been used for a while and may cause confusion. However, since we have full control of ztunnel, it may be fine to have a different behavior than Envoy? @GregHanson Do you have any suggestions? |
I wouldn't spend so much time on this...
…On Tue, Aug 15, 2023 at 12:10 AM Xiaopeng Han ***@***.***> wrote:
I think the better solution is to have envoy and ztunnel accept GET
requests. If we just want to mirror behavior, we can replace the hyperlink
with a button.
@Stevenjin8 <https://github.com/Stevenjin8> I think adding a GET request
to Envoy may not be acceptable since POST has been used for a while and may
cause confusion. However, since we have full control of ztunnel, it may be
fine to have a different behavior than Envoy? @GregHanson
<https://github.com/GregHanson> Do you have any suggestions?
—
Reply to this email directly, view it on GitHub
<#638 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXKPEMDZNKXNHR2PEQ3XVMOHZANCNFSM6AAAAAA3DVXV5Y>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@hanxiaop @Stevenjin8 I don't like the idea of modifying the API to accept GET requests. I envisioned this one would require some html magic to get a page more similar to what envoy has (i.e. buttons for POST etc). This issue encompassed a few items of varying degrees of importance IMO.
Agreeing with John that the remaining item is lower priority. This issue could be used for future tracking - or we can mark it as |
Ref: istio/istio#45695 (comment)
/config_dump
does not return a pretty printed json object/logging
is unusable since it expects a POST and a level, this should be similar to envoy logging API./stats
API on 15000, but it does have one on :15020/metrics
The text was updated successfully, but these errors were encountered: