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

Apply MQE on K8s layer UI-templates #11350

Merged
merged 4 commits into from
Sep 24, 2023
Merged

Conversation

smartboy37597
Copy link
Contributor

@smartboy37597 smartboy37597 commented Sep 22, 2023

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10914
  • Update the CHANGES log.

Here are some UI and e2e test screenshots.
ui-5
ui-3
k8su1-2
k8s-ui
k8se2e-success

@wu-sheng wu-sheng added this to the 9.7.0 milestone Sep 22, 2023
@wu-sheng wu-sheng added backend OAP backend related. enhancement Enhancement on performance or codes labels Sep 22, 2023
skywalking-ui Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems an old version. A mistake sync?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be a mistake. @smartboy37597 Please revert it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be a sync error, I will try to solve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ui is synchronized

@wu-sheng
Copy link
Member

ebpf cases have errors. Something is not matched. cc @mrproliu

- metric:
labels:
- key: _
value: "false-http-172.18.0.6:80:50"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the value of the label look like a dynamic IP?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel different KinD would provide different IP pools. So, don't use the specific addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process_relation_client_write_rtt_time_percentile and process_relation_client_write_exe_time_percentile
process_relation_server_write_rtt_time_percentile and process_relation_server_write_exe_time_percentile
Can the metrics return the same value as process_relation_http1_request_package_size_percentile and process_relation_http1_response_package_size_percentile is consistent, and I think the label value of http1 is better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How TCP could be as same as HTTP?
This PR should keep a metric to MQE, let's don't introduce more changes in here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is enough for you to verify that the value of the label is not empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smartboy37597 I will approve the rerun once you fix the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it and the e2e test are passing.
image

@wu-sheng
Copy link
Member

I will take care of Dead Link Checker through another PR. It seems some links had been removed from Envoy doc site.

@@ -1,19 +1,3 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kezhenxu94 Should we keep the license header in the UI template files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we no longer need these headers: #11184 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smartboy37597 Could you follow up on another PR to remove other existing headers to keep them consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smartboy37597 Could you follow up on another PR to remove other existing headers to keep them consistent?

Okey, I'm going to delete all UI headers in a new PR.

Copy link
Member

@wankai123 wankai123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wu-sheng wu-sheng requested a review from mrproliu September 24, 2023 01:29
@wu-sheng wu-sheng merged commit 7ff7d73 into apache:master Sep 24, 2023
161 of 162 checks passed
@wu-sheng
Copy link
Member

@smartboy37597 We only have OpenFunction and topology related settings pending on the immigration to MQE.
Take your time to finish the final step.

@smartboy37597
Copy link
Contributor Author

@smartboy37597 We only have OpenFunction and topology related settings pending on the immigration to MQE. Take your time to finish the final step.

Okey, i got it.

liangyepianzhou pushed a commit to liangyepianzhou/skywalking that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. enhancement Enhancement on performance or codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants