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

Documenting inter-zone traffic #1696

Merged
merged 4 commits into from
Feb 27, 2025
Merged

Conversation

mariomac
Copy link
Contributor

Addresses #1610

@mariomac mariomac requested a review from grafsean as a code owner February 24, 2025 11:50
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.81%. Comparing base (ee96e94) to head (c19bc7f).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1696      +/-   ##
==========================================
+ Coverage   69.67%   69.81%   +0.14%     
==========================================
  Files         209      209              
  Lines       21006    21065      +59     
==========================================
+ Hits        14635    14707      +72     
+ Misses       5664     5644      -20     
- Partials      707      714       +7     
Flag Coverage Δ
integration-test 54.02% <ø> (-0.15%) ⬇️
k8s-integration-test 54.30% <ø> (+0.07%) ⬆️
oats-test 35.12% <ø> (+0.20%) ⬆️
unittests 46.24% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mariomac mariomac requested a review from a team February 24, 2025 13:26
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Minor wording change requested.


## Use the `beyla.network.inter.zone` metric

Using a separate metric for inter-zone traffic allows measuring it with a reduced impact in the metrics cardinality,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd reword this paragraph as this, Sean might have a better version :) :

Using a separate metric for inter-zone traffic reduces the metric cardinality impact of collecting this data,
because the src.zone and dst.zone attributes are not added to the regular network metrics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Good suggestion 👍

@marevers
Copy link
Collaborator

Fantastic. This is a really great addition! LGTM.

Copy link
Contributor

@grafsean grafsean left a comment

Choose a reason for hiding this comment

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

Minor changes suggested.

# Measure traffic between Cloud availability zones

{{< admonition type="note" >}}
Currently, this feature is only available in Kubernetes clusters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Currently, this feature is only available in Kubernetes clusters.
This feature is currently only available in Kubernetes clusters.


## Use the `beyla.network.inter.zone` metric

Using a separate metric for inter-zone traffic allows measuring it with a reduced impact in the metrics cardinality,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good suggestion 👍

@mariomac mariomac merged commit 241e11f into grafana:main Feb 27, 2025
14 of 15 checks passed
@mariomac mariomac deleted the inter-zone-docs branch February 27, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants