Skip to content

Support monitoring AWS Cloud EKS#10199

Merged
wu-sheng merged 14 commits into
apache:masterfrom
pg-yang:aws-moniting-v2
Dec 31, 2022
Merged

Support monitoring AWS Cloud EKS#10199
wu-sheng merged 14 commits into
apache:masterfrom
pg-yang:aws-moniting-v2

Conversation

@pg-yang

@pg-yang pg-yang commented Dec 25, 2022

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

Related issue #9883

  1. UI repo has synced
  2. I copied otlp proto to java-test-service for mocking EKS e2e test

UI screenshot

image

image

Node metrics

image

image

Service metrics

image

@pg-yang pg-yang requested a review from wu-sheng December 25, 2022 10:06
@pg-yang pg-yang added this to the 9.4.0 milestone Dec 25, 2022
@pg-yang pg-yang added the backend OAP backend related. label Dec 25, 2022
@wu-sheng wu-sheng added metrics Metrics and meter ecosystem Things related to SkyWalking, but codes hosting in 3rd-party feature New feature labels Dec 25, 2022
@wu-sheng wu-sheng requested a review from wankai123 December 25, 2022 11:10
Comment thread docs/en/setup/backend/backend-aws-eks-monitoring.md
- name: net_tx_dropped
exp: node_network_tx_dropped.downsampling(SUM)
- name: net_tx_error
exp: node_network_tx_errors.downsampling(SUM)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you confirm why these metrics use downsampling(SUM), I can see these units are Count/Second from the
origin OTEL metrics. SUM would plus the value of the metrics each time, should use the default downsampling
AVG?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please ignore the approval for now... It's a mistake.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sum may be not correct, especially at high dimensiona(hour/day)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pg-yang Could you check here? Network TX Error Count (per second) is the description, why it could be downsampling(SUM)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, I will confirm these issues by the end of this week at the latest.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it is.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I couldn't find any documentation that describes the calculation, but I found the source code.
In a word, the calculation logic is (preValue-currentValue) / (preTimeInSecond-currentTimeInSecond).

  1. value (preValue or currentValue) is cumulative count of transmit errors encountered. It's defined in https://github.com/google/cadvisor/blob/master/info/v1/container.go#L434
  2. currentValue, currentTimeInSecond will be stored to cache as preValue, preTimeInSecond for the next calculation

I put the source code here:

  1. Calculate method: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/951245d4273845de59db3b00f5445bcad9ce4372/internal/aws/metrics/metric_calculator.go#L67
  2. The calculateFunc method that is called by Calculate method: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/951245d4273845de59db3b00f5445bcad9ce4372/receiver/awscontainerinsightreceiver/internal/cadvisor/extractors/extractor.go#L127

BTW

  - name: net_tx_error
    exp: node_network_tx_errors.downsampling(SUM)

The above metric is organized from node level to cluster level,so I think latest should be inappropriate.Also,if we use avg, the meaning will be net errors per second, per node.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

From your description, I would say avg makes more sense.
Sum for day and hour dimension would be a disaster, right?
We have to keep per node, I am afraid, there is no way we could sum first and then avg, because from data process perspective, there is no difference between two node and two period report of one node, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

image

wu-sheng
wu-sheng previously approved these changes Dec 27, 2022

@wu-sheng wu-sheng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Pending on more confirmations from @kezhenxu94 @wankai123


### AWS Container Insights Receiver Deploy Example

```yaml

@kezhenxu94 kezhenxu94 Dec 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sample yaml is way too long that it doesn't highlight the important part, please remove unrelated content and replace with their official documentation link, an example would be

# ... other resources definitions
# OTEL configuration that specify OAP address("oap-service:11800") as otlp exporter address
apiVersion: v1
kind: ConfigMap
metadata:
  name: otel-agent-conf
  namespace: aws-otel-eks
  labels:
    app: opentelemetry
    component: otel-agent-conf
data:
  otel-agent-config: |
    # ...
    receivers:
      awscontainerinsightreceiver:

    # Make sure to add a job-name attribute
    processors:
      resource/job-name:
        attributes:
        - key: job_name   
          value: aws-cloud-eks-monitoring
          action: insert     

    exporters:
      otlp:
        endpoint: oap-service:11800
        tls:
          insecure: true
      logging:
          loglevel: debug          

    service:
      pipelines:
        metrics:
          receivers: [awscontainerinsightreceiver]
          processors: [resource/job-name]
          exporters: [otlp,logging]

# ... other resources

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you want to give a complete sample, let's create a dedicated yaml file and link to that file, so users can simply use kubectl apply -f sample.yaml to set up the sample, without having to copy the large chunk of content.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think a complete OTEL YAML file would be good to host in the doc. Or we could link the file to the incoming file in the showcase.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think a complete OTEL YAML file would be good to host in the doc

Agree with it, we don't have showcase for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@pg-yang Are you going to move this to a separate file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I only provide a OTEL configuration sample in the doc

kezhenxu94
kezhenxu94 previously approved these changes Dec 28, 2022

@kezhenxu94 kezhenxu94 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A nit, otherwise LGTM

@wu-sheng

Copy link
Copy Markdown
Member

@pg-yang Let's fix the left issues and merge this :)

@pg-yang pg-yang dismissed stale reviews from kezhenxu94 and wu-sheng via cf5911a December 31, 2022 08:16

@wu-sheng wu-sheng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wankai123 wankai123 left a comment

Copy link
Copy Markdown
Member

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 merged commit 8de8c7c into apache:master Dec 31, 2022
@pg-yang pg-yang deleted the aws-moniting-v2 branch December 31, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related. ecosystem Things related to SkyWalking, but codes hosting in 3rd-party feature New feature metrics Metrics and meter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants