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

[Metricbeat] Add Airflow module in xpack #26220

Merged
merged 22 commits into from
Jul 1, 2021
Merged

[Metricbeat] Add Airflow module in xpack #26220

merged 22 commits into from
Jul 1, 2021

Conversation

aspacca
Copy link

@aspacca aspacca commented Jun 9, 2021

What does this PR do?

Adding Airflow monitoring as lightweight module based on Statsd (https://airflow.apache.org/docs/apache-airflow/1.10.15/metrics.html)
Introduced statsd.mapping to filter and transform Airflow mapping (it's anyway generic and could be applied to any other Statsd lightweigth module in the future).

It uses eventMapping(...) function according to https://github.com/elastic/beats/blob/23e4403ae093fcc8f7905345cad2c7ad256976d8/docs/devguide/metricset-details.asciidoc#data-transformation. It is anyway in the Run() method and not in the Fetch() one, but there is where we call reporter.Event(*e) for the module. It has to be clarified if it's correct.

Why is it important?

Because it is my first PR :)

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • 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 works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

$ ./metricbeat modules enable airflow
$ ./metricbeat -e # (Start metricbeat according your preferred setup)
$ echo "dagrun.duration.failed.dagid:200|ms" > /dev/udp/127.0.0.1/8126 # (with any of the metrics that can be found at https://airflow.apache.org/docs/apache-airflow/1.10.15/metrics.html)

Related issues

Closes #26197

Use cases

  Scenario: Andrea monitors Airflow
    Given the Andrea has an Airflow instance to monitor
    When the Airflow module is enabled
    And statsd configuration points to it in Airflow
    Then Andrea would be able to see Airflow metrics in Kibana

Screenshots

Logs

2021-06-08T18:08:47.126+0200    INFO    udp/udp.go:80   Started listening for UDP on: 127.0.0.1:8126

@aspacca aspacca added enhancement Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Jun 9, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 9, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 9, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #26220 updated

  • Start Time: 2021-07-01T07:59:45.915+0000

  • Duration: 94 min 45 sec

  • Commit: 2b32fdc

Test stats 🧪

Test Results
Failed 0
Passed 9473
Skipped 2595
Total 12068

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 9473
Skipped 2595
Total 12068

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Great first PR @aspacca! I think the new mapping support in the statsd module offers many possibilities. I have added some comments, but mostly about Metricbeat specifics.

This change will need two changelog entries, one for the new mapping in the statsd module and another one for the new airflow module. And this makes me think that this PR might be splitted into two, this will make it easier to separate the specific discussions for each feature, and we can focus first in merging the new mapping for the statsd module. What do you think?

x-pack/metricbeat/module/airflow/_meta/Dockerfile Outdated Show resolved Hide resolved
x-pack/metricbeat/module/airflow/_meta/Dockerfile Outdated Show resolved Hide resolved
x-pack/metricbeat/module/airflow/statsd/_meta/data.json Outdated Show resolved Hide resolved
x-pack/metricbeat/module/airflow/statsd/_meta/data.json Outdated Show resolved Hide resolved
x-pack/metricbeat/module/airflow/statsd/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/server.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data.go Outdated Show resolved Hide resolved
@aspacca
Copy link
Author

aspacca commented Jun 10, 2021

This change will need two changelog entries, one for the new mapping in the statsd module and another one for the new airflow module.

just adding the entries under Metricbeat in CHANGELOG.next.asciidoc, is it?

And this makes me think that this PR might be splitted into two, this will make it easier to separate the specific discussions for each feature, and we can focus first in merging the new mapping for the statsd module. What do you think?

I'm fine having both discussions in this PR, but I understand your "concern", it's up about what is the standard for such cases in the team.
in case the second PR should be based on this branch (removing the part strictly related to Airflow), is it fine?

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks for addressing most of the comments! Adding some extra suggestions.

x-pack/metricbeat/module/airflow/_meta/fields.yml Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data_test.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data_test.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/server.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/airflow/_meta/Dockerfile Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-airflow-module upstream/add-airflow-module
git merge upstream/master
git push upstream add-airflow-module

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Added some minor comments, nothing serious. Good job!

x-pack/metricbeat/module/statsd/server/server.go Outdated Show resolved Hide resolved
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data_test.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/data_test.go Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member

/test

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the comments! I have added some extra nitpicking, but I'd say this is ready to merge.

x-pack/metricbeat/module/airflow/statsd/data_test.go Outdated Show resolved Hide resolved
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/server.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/statsd/server/server.go Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member

One failing test seems related, the other one is unrelated (#26537).

@aspacca
Copy link
Author

aspacca commented Jun 29, 2021

on win images windows firewall seems to block the udp statsd server created by the module. that's the reason why the TestData is failing.

@jsoriano
Copy link
Member

on win images windows firewall seems to block the udp statsd server created by the module. that's the reason why the TestData is failing.

Ok, then skip these tests in windows by now, and maybe in the future we can configure the firewall in these images to allow local UDP connections.

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-airflow-module upstream/add-airflow-module
git merge upstream/master
git push upstream add-airflow-module

@jsoriano jsoriano added the backport-v7.14.0 Automated backport with mergify label Jun 30, 2021
@mergify
Copy link
Contributor

mergify bot commented Jul 1, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b add-airflow-module upstream/add-airflow-module
git merge upstream/master
git push upstream add-airflow-module

@aspacca aspacca merged commit 3d01b5b into elastic:master Jul 1, 2021
mergify bot pushed a commit that referenced this pull request Jul 1, 2021
* [Metricbeat] Add Airflow module

Airflow module is a lightweight module based on Statsd module.
It filters, renames and adds metrics according to mappings defined
in the manifest.yml

(cherry picked from commit 3d01b5b)
@aspacca aspacca added backport-v7.15.0 Automated backport with mergify and removed backport-v7.14.0 Automated backport with mergify labels Jul 5, 2021
mergify bot pushed a commit that referenced this pull request Jul 5, 2021
* [Metricbeat] Add Airflow module

Airflow module is a lightweight module based on Statsd module.
It filters, renames and adds metrics according to mappings defined
in the manifest.yml

(cherry picked from commit 3d01b5b)
v1v added a commit to v1v/beats that referenced this pull request Jul 5, 2021
…stage-failed-within-same-build

* upstream/master: (36 commits)
  Revert "[CI] fight the flakiness with some retry option in the CI only for the Pull Requests (elastic#26617)" (elastic#26704)
  Packaging: linux/armv7 is not supported (elastic#26706)
  Cyberarkpas: Link to official docs on how to setup TLS (elastic#26614)
  Make network_direction, registered_domain and convert processors compatible with ES older than 7.13.0 (elastic#26676)
  Disable armv7 packaging (elastic#26679)
  [Heartbeat] use --params flag for synthetics (elastic#26674)
  Update dependent package to avoid downloading a suspicious file (elastic#26406)
  [mergify] set title and allow bp in any direction (elastic#26648)
  Fix memory leak in SQL helper when database is not available (elastic#26607)
  [CI] fight the flakiness with some retry option in the CI only for the Pull Requests (elastic#26617)
  [mergify] automate PRs that change the backport rules (elastic#26641)
  [Metricbeat] Add Airflow module in xpack (elastic#26220)
  chore: add-backport-next (elastic#26620)
  [metricbeat] Add state_job metricset (elastic#26479)
  CI: jenkins labels are less time consuming now (elastic#26613)
  [MetricBeat] [AWS] Fix aws metric tags with resourcegroupstaggingapi paginator (elastic#26385) (elastic#26443)
  Move openmetrics module to oss (elastic#26561)
  Skip flaky test TestFilestreamMetadataUpdatedOnRename (elastic#26609)
  [filebeat][fortinet] Use default add_locale for fortinet.firewall (elastic#26524)
  Enroll proxy settings (elastic#26514)
  ...
aspacca pushed a commit that referenced this pull request Jul 6, 2021
* [Metricbeat] Add Airflow module

Airflow module is a lightweight module based on Statsd module.
It filters, renames and adds metrics according to mappings defined
in the manifest.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.15.0 Automated backport with mergify enhancement Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat] Add Airflow module in xpack
3 participants