Allow for easier network metrics enablement #1013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
+ Coverage 80.18% 80.23% +0.04%
==========================================
Files 137 137
Lines 10932 10942 +10
==========================================
+ Hits 8766 8779 +13
+ Misses 1685 1683 -2
+ Partials 481 480 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mariomac
approved these changes
Jul 15, 2024
Comment on lines
52
to
+57
| | -------- | ----------------------- | ------- | ------- | | ||
| | `enable` | `BEYLA_NETWORK_METRICS` | boolean | `false` | | ||
|
|
||
| Enables network metrics reporting in Beyla. | ||
| Explicitly enables network metrics reporting in Beyla. You can also enable network metrics reporting | ||
| by adding `network` to the list of `features` for [otel_metrics_export]({{< relref "../configure/options.md#otel-metrics-exporter" >}})) | ||
| or [prometheus_export]({{< relref "../configure/options.md#prometheus-http-endpoint" >}})). |
There was a problem hiding this comment.
If we are going to keep this only for backwards-compatibility purposes, I'd even delete it from the documentation.
There was a problem hiding this comment.
I think I should follow-up with a PR to remove the docs and also remove it from examples and the chart. Perhaps we do this work for 2.0?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enabling network metrics could be confusing if the documentation isn't always properly read. Essentially we had an internal default of enabled metrics features "application,network", however to actually enable network metrics collection we had to explicitly turn on network monitoring. This duality created some friction. For example, we'd fail to enable properly network metrics with these configuration options:
If we read what the configuration says:
A properly working configuration would've been:
So kind of double enabling network metrics.
At the same time, the following configuration doesn't enable network metrics:
Since we are missing the flag to enable network metrics, even though we've specified in the features we want network metrics.
I think the correct way to enable any metric collection feature is to list it explicitly in the
featuressection, e.g. "application,network" should enable both, no further option should be required.This PR resolves this, but to keep backwards compatibility I kept the explicit enablement of the network metrics via BEYLA_NETWORK_METRICS: "true", which is equivalent to adding the "network" keyword in the
featuressection for both OTel and Prometheus export.