Adding support for zipkin UI #127

Yangfisher1 · 2023-07-24T07:13:45Z

Marking the variable SW_ZIPKIN_ADDRESS explicitly for deploying the zipkin UI more easily on k8s cluster.

innerpeacez

I think just configuring the ui.env in the values.yaml

kezhenxu94 · 2023-07-24T07:43:11Z

chart/skywalking/values.yaml

@@ -41,6 +41,7 @@ oap:
    # zabbix: 10051
    grpc: 11800
    rest: 12800
+    zipkinUI: 9412


I think this should not be enabled by default?

Ok. Maybe I should if to create a conditional block in ui-deployment.yaml.

Yangfisher1 · 2023-07-24T07:45:24Z

I think using these variables in Chart template is easier especially for those who are not familiar with Helm. But at least adding some guide about how to deploy the zipkin UI on k8s cluster is needed. It took me a lot of time to find out the variable SW_ZIPKIN_ADDRESS should be set.

Yangfisher1 · 2023-07-24T12:36:12Z

Use ui.zipkin.enabled and oap.port.zipkinui to control whether set the variable SW_ZIPKIN_ADDRESS or not.

kezhenxu94

@Yangfisher1 the configurations look verbose to me, here is what I think:

Always add the SW_ZIPKIN_ADDRESS, there is no side effect for this env var to be set no matter Zipkin plugin is enabled or disabled.
Conditionally add the exposed port 9412 in the OAP service: when the SW_QUERY_ZIPKIN is set to default, add the port 9412 to service OAP, otherwise, don't add the port.
Remove oap.ports.zipkinui and zipkin.enabled, because that's not the way to enable/disable zipking plugin, it's SW_QUERY_ZIPKIN that does this.

@Yangfisher1 let me know what you think

Yangfisher1 · 2023-07-24T13:02:13Z

@kezhenxu94

Conditionally add the exposed port 9412 in the OAP service: when the SW_QUERY_ZIPKIN is set to default, add the port 9412 to service OAP, otherwise, don't add the port.

I think this idea is better.

Always add the SW_ZIPKIN_ADDRESS, there is no side effect for this env var to be set no matter Zipkin plugin is enabled or disabled.

However, there's one side effect for this env var to be set without setting the port. The UI pod will throw an error complaining about the missing port when initializing. I think we should use the env var SW_QUERY_ZIPKIN to control whether the SW_ZIPKIN_ADDRESS is set or not. Is it a good idea?

kezhenxu94 · 2023-07-24T13:03:26Z

However, there's one side effect for this env var to be set without setting the port. The UI pod will throw an error complaining about the missing port when initializing. I think we should use the env var SW_QUERY_ZIPKIN to control whether the SW_ZIPKIN_ADDRESS is set or not. Is it a good idea?

Right I missed that, that's better!

Yangfisher1 · 2023-07-24T14:58:21Z

@kezhenxu94

It works well now. But I think directly hardcoding the port number(like 9141 and 9142) in the file is not very graceful.

wu-sheng · 2023-07-24T15:05:56Z

chart/skywalking/templates/oap-deployment.yaml

+
+{{- if eq .Values.oap.env.SW_QUERY_ZIPKIN "default" }}
+        - containerPort: 9412
+          name: "zipkin-ui"


FYI @jcchavezs Another new Zipkin is being added.

@wu-sheng I think it's the original Zipkin bounded in Skywalking OAP Server. We just configure the deployment, making it easier to use Zipkin trace in Skywalking on k8s cluster since I find there're some issues about the configuration of Zipkin.

Yes, but it was not exposed I think.

This is for the sake of convenience, nothing new is added, nothing old was blocked without this changes

kezhenxu94 · 2023-07-25T05:09:04Z

@kezhenxu94

It works well now. But I think directly hardcoding the port number(like 9141 and 9142) in the file is not very graceful.

I agree, but I don't thin we have a more graceful way to do this, do you have any better idea, I'm open to discuss?

innerpeacez · 2023-07-25T06:37:25Z

values.yaml

oap:
  ports:
    # add more ports here if you need, for example
    # zabbix: 10051
    grpc: 11800
    rest: 12800
    # zipkinReceiver: 9411
    # zipkinQuery: 9412

oap-deployment.yaml

        env:
{{- if .Values.oap.ports.zipkinReceiver }}
        - name: SW_RECEIVER_ZIPKIN
          value: "default"
        - name: SW_RECEIVER_ZIPKIN_REST_PORT
          value: "{{ .Values.oap.ports.zipkinReceiver }}"
{{- end }}
{{- if .Values.oap.ports.zipkinQuery }}
        - name: SW_QUERY_ZIPKIN
          value: "default"
        - name: SW_QUERY_ZIPKIN_REST_PORT
          value: "{{ .Values.oap.ports.zipkinReceiver }}"
{{- end }}

ui-deployment.yaml

        env:
{{- if .Values.oap.ports.zipkinQuery }}
        - name: SW_ZIPKIN_ADDRESS
          value: "http://{{ template "skywalking.oap.fullname" . }}:{{ .Values.oap.ports.zipkinQuery }}"
{{- end }}

I think it might be more convenient. What do you think? @kezhenxu94 @Yangfisher1

kezhenxu94 · 2023-07-25T06:42:43Z

values.yaml

oap:
  ports:
    # add more ports here if you need, for example
    # zabbix: 10051
    grpc: 11800
    rest: 12800
    # zipkinReceiver: 9411
    # zipkinQuery: 9412

oap-deployment.yaml

        env:
{{- if .Values.oap.ports.zipkinReceiver }}
        - name: SW_RECEIVER_ZIPKIN
          value: "default"
{{- end }}
{{- if .Values.oap.ports.zipkinQuery }}
        - name: SW_QUERY_ZIPKIN
          value: "default"
{{- end }}

ui-deployment.yaml

        env:
{{- if .Values.oap.ports.zipkinQuery }}
        - name: SW_ZIPKIN_ADDRESS
          value: "http://{{ template "skywalking.oap.fullname" . }}:{{ .Values.oap.ports.zipkinQuery }}"
{{- end }}

I think it might be more convenient. What do you think? @kezhenxu94 @Yangfisher1

This works for me too, another good point is that we can also set SW_QUERY_ZIPKIN_REST_PORT to .Values.ports.zipkinQuery, @Yangfisher1 in that case users can set Zipin port to arbitrary port, I think

Yangfisher1 · 2023-07-25T07:14:42Z

@innerpeacez @kezhenxu94 It works for me, making it more convenient and graceful. Users only need to configure these two ports to enable zipkin trace and they can choose any port they want.

I'd like to make these changes and add these ports into README.md.

chart/skywalking/templates/oap-deployment.yaml

innerpeacez

LGTM

Yangfisher1 · 2023-07-25T08:40:13Z

@innerpeacez @kezhenxu94
I tested it on my k8s cluster and it worked well. Since helm only supports lowercase letters so I rename these ports.

innerpeacez · 2023-07-25T08:56:35Z

Since helm only supports lowercase letters so I rename these ports.

Camel-Case is supported, let's keep it. ref to https://helm.sh/zh/docs/chart_best_practices/values/

Yangfisher1 · 2023-07-25T09:02:39Z

@innerpeacez My bad. It's the restriction of Kubernetes as a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

innerpeacez · 2023-07-25T09:22:34Z

@innerpeacez My bad. It's the restriction of Kubernetes as a lowercase RFC 1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

Oooops! For more convenience. Named zkreceiver zkquery, to avoid using gotemplate index function. You think? @Yangfisher1

Yangfisher1 · 2023-07-25T10:05:55Z

@innerpeacez
I'm ok with this. But I'm concerned that using zk instead of zipkin might be confusing. Using 'index' would ensure the readability of these ports.

kezhenxu94 · 2023-07-25T10:24:02Z

I vote for not to use index function, using . is also friendly for completion in IDEs。

kezhenxu94 · 2023-07-25T10:24:43Z

@innerpeacez

I'm ok with this. But I'm concerned that using zk instead of zipkin might be confusing. Using 'index' would ensure the readability of these ports.

We can use zipkinreceiver or zipkinquery?

Yangfisher1 · 2023-07-25T10:30:17Z

@kezhenxu94

We can use zipkinreceiver or zipkinquery?

I'm ok with this.

innerpeacez · 2023-07-25T10:31:18Z

@innerpeacez

I'm ok with this. But I'm concerned that using zk instead of zipkin might be confusing. Using 'index' would ensure the readability of these ports.

We can use zipkinreceiver or zipkinquery?

It's better. I think.

Yangfisher1 · 2023-07-25T10:58:15Z

@kezhenxu94 @innerpeacez
Renaming is done.

kezhenxu94

Thanks for your patience @Yangfisher1 🙇

wu-sheng · 2023-07-25T13:49:18Z

Congrats @Yangfisher1 to be 800th GitHub contributor of SkyWalking.
Welcome.

Yangfisher1 · 2023-07-25T14:10:04Z

@kezhenxu94 @innerpeacez @wu-sheng Thank you for your help! This is a wonderful community and everyone is kind🤩

innerpeacez reviewed Jul 24, 2023

View reviewed changes

innerpeacez requested review from kezhenxu94 and wu-sheng July 24, 2023 07:38

kezhenxu94 reviewed Jul 24, 2023

View reviewed changes

Yangfisher1 force-pushed the master branch from 56e0843 to 6b39624 Compare July 24, 2023 12:33

Yangfisher1 requested a review from kezhenxu94 July 24, 2023 12:38

kezhenxu94 reviewed Jul 24, 2023

View reviewed changes

Yangfisher1 force-pushed the master branch from 6b39624 to 3992197 Compare July 24, 2023 14:54

wu-sheng reviewed Jul 24, 2023

View reviewed changes

kezhenxu94 approved these changes Jul 25, 2023

View reviewed changes

Yangfisher1 force-pushed the master branch from eae11c5 to 6a4803b Compare July 25, 2023 08:06

innerpeacez reviewed Jul 25, 2023

View reviewed changes

chart/skywalking/templates/oap-deployment.yaml Outdated Show resolved Hide resolved

innerpeacez reviewed Jul 25, 2023

View reviewed changes

chart/skywalking/templates/oap-deployment.yaml Outdated Show resolved Hide resolved

innerpeacez approved these changes Jul 25, 2023

View reviewed changes

Yangfisher1 force-pushed the master branch from 6b4a7de to eb36233 Compare July 25, 2023 08:38

Yangfisher1 force-pushed the master branch from eb36233 to dfe30e2 Compare July 25, 2023 08:42

innerpeacez approved these changes Jul 25, 2023

View reviewed changes

Adding support for zipkin UI

881b2c7

Yangfisher1 force-pushed the master branch from dfe30e2 to 881b2c7 Compare July 25, 2023 10:56

innerpeacez approved these changes Jul 25, 2023

View reviewed changes

kezhenxu94 approved these changes Jul 25, 2023

View reviewed changes

innerpeacez merged commit 73f5788 into apache:master Jul 25, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for zipkin UI #127

Adding support for zipkin UI #127

Yangfisher1 commented Jul 24, 2023

innerpeacez left a comment

kezhenxu94 Jul 24, 2023

Yangfisher1 Jul 24, 2023

Yangfisher1 commented Jul 24, 2023

Yangfisher1 commented Jul 24, 2023 •

edited

kezhenxu94 left a comment

Yangfisher1 commented Jul 24, 2023

kezhenxu94 commented Jul 24, 2023

Yangfisher1 commented Jul 24, 2023

wu-sheng Jul 24, 2023

Yangfisher1 Jul 25, 2023

wu-sheng Jul 25, 2023

kezhenxu94 Jul 25, 2023

kezhenxu94 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023 •

edited

kezhenxu94 commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez left a comment

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

kezhenxu94 commented Jul 25, 2023

kezhenxu94 commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

kezhenxu94 left a comment

wu-sheng commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

Adding support for zipkin UI #127

Adding support for zipkin UI #127

Conversation

Yangfisher1 commented Jul 24, 2023

innerpeacez left a comment

Choose a reason for hiding this comment

kezhenxu94 Jul 24, 2023

Choose a reason for hiding this comment

Yangfisher1 Jul 24, 2023

Choose a reason for hiding this comment

Yangfisher1 commented Jul 24, 2023

Yangfisher1 commented Jul 24, 2023 • edited

kezhenxu94 left a comment

Choose a reason for hiding this comment

Yangfisher1 commented Jul 24, 2023

kezhenxu94 commented Jul 24, 2023

Yangfisher1 commented Jul 24, 2023

wu-sheng Jul 24, 2023

Choose a reason for hiding this comment

Yangfisher1 Jul 25, 2023

Choose a reason for hiding this comment

wu-sheng Jul 25, 2023

Choose a reason for hiding this comment

kezhenxu94 Jul 25, 2023

Choose a reason for hiding this comment

kezhenxu94 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023 • edited

kezhenxu94 commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez left a comment

Choose a reason for hiding this comment

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

kezhenxu94 commented Jul 25, 2023

kezhenxu94 commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

innerpeacez commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

kezhenxu94 left a comment

Choose a reason for hiding this comment

wu-sheng commented Jul 25, 2023

Yangfisher1 commented Jul 25, 2023

Yangfisher1 commented Jul 24, 2023 •

edited

innerpeacez commented Jul 25, 2023 •

edited