-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Log error when statsd server fails to start and fix airflow docs #36477
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
Looks good to me but I could see more adjustments that could be made to Airflow docs. Sample patch: This module collects metrics from
-https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/metrics.html[Airflow metrics]. It runs a
-statsd server where airflow will send metrics to. The default metricset is `statsd`.
+https://airflow.apache.org/docs/apache-airflow/stable/logging-monitoring/metrics.html[Airflow metrics]. It runs a statsd server where airflow will send metrics to. The default metricset is `statsd`.
[float]
=== Compatibility
-The Airflow module is tested with Airflow 2.1.0. It should work with version
-2.0.0 and later.
+The Airflow module is tested with Airflow 2.1.0. It should work with version 2.0.0 and later.
[float]
=== Usage
-The Airflow module requires <<metricbeat-module-statsd,Statsd>> to receive Statsd metrics. Refer to the link for instructions about how to use Statsd.
+The Airflow module requires <<metricbeat-module-statsd,Statsd>> to receive statsd metrics. Refer to the link for instructions about how to use statsd.
-Add the following lines to your Airflow configuration file e.g. `airflow.cfg` ensuring `statsd_prefix` is left empty and replace `%METRICBEAT_HOST%` with the address metricbeat is running:
+Add the following lines to your Airflow configuration file e.g. `airflow.cfg` ensuring `statsd_prefix` is left empty and replace `%METRICBEAT_HOST%` with the address where metricbeat is running: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment. Also, CI is failing. Could you please also run make update
?
This commit adds a log error when the statsd server fails to start, it also improves the Airflow module documentation to make extra clear that the module will start a statsd server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: rename the method receiver from b
to m
: s/b/m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not important though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bigger change on the this file, I rather make it into a separated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The followup PR: #36591
I think when I first looked I was thinking about the opposite change 🤦♂️. It's a super small PR :D
) This commit adds a log error when the statsd server fails to start, it also improves the Airflow module documentation to make extra clear that the module will start a statsd server. Co-authored-by: subham sarkar <[email protected]> (cherry picked from commit 9dcd8be)
…stic#36477) This commit adds a log error when the statsd server fails to start, it also improves the Airflow module documentation to make extra clear that the module will start a statsd server. Co-authored-by: subham sarkar <[email protected]>
Proposed commit message
This commit adds a log error when the statsd server fails to start, it
also improves the Airflow module documentation to make extra clear
that the module will start a statsd server.
Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files- [ ] I have added tests that prove my fix is effective or that my feature worksCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.## Author's ChecklistHow to test this PR locally
metricbeat.yml
:## Related issues## Use cases## Screenshots## Logs