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

Update mapping and tags in README.md #11967

Merged
merged 6 commits into from
Jun 6, 2022
Merged

Conversation

moshederri
Copy link
Contributor

Adjusting airflow matching pattern to include missing airflow., as well as unifying tags for dag and task id (dag_id / task_id) as they were different between mapping

What does this PR do?

Fix invalid mappings as well as unify tags

Motivation

Using the previous version did not match all metrics as well as forced us to use two different template variables (for example, dagid and dag_id)

Adjusting airflow matching pattern to include missing `airflow.`, as well as unifying tags for dag and task id (dag_id / task_id) as they were different between mapping
@moshederri moshederri requested review from a team as code owners May 9, 2022 17:10
@moshederri moshederri changed the title Update README.md Update mapping and tags in README.md May 9, 2022
kayayarai
kayayarai previously approved these changes May 9, 2022
@hithwen
Copy link
Contributor

hithwen commented May 10, 2022

Hallo, thanks for your contribution. Can you update it on line 311 too?

name: "airflow.scheduler.tasks.starving"
- match: sla_email_notification_failure
- match: airflow.sla_email_notification_failure
Copy link
Contributor

Choose a reason for hiding this comment

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

should this match also be wrapped in quotes?

Suggested change
- match: airflow.sla_email_notification_failure
- match: 'airflow.sla_email_notification_failure'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thank you, updated!

@@ -108,11 +108,11 @@ Connect Airflow to DogStatsD (included in the Datadog Agent) by using the Airflo
name: "airflow.pool.open_slots"
tags:
pool_name: "$1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, hope i got the last one right though

@codecov
Copy link

codecov bot commented May 14, 2022

Codecov Report

Merging #11967 (a601701) into master (4d971a2) will increase coverage by 0.06%.
The diff coverage is n/a.

Flag Coverage Δ
airflow 90.00% <ø> (ø)
teradata ?

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

@moshederri
Copy link
Contributor Author

@sarah-witt @fanny-jiang is there anything else you would like me to change? by the way, regardless to this change, i think that you are missing documentation on how to properly configure dd agent to collect airflow worker's tasks logs in Kubernetes (specific annotation, mounting volume and a new log pipeline).

@yzhan289
Copy link
Contributor

yzhan289 commented Jun 2, 2022

Hi @moshederri there seems to be a small merge conflict. We can approve and merge after resolving this merge conflict.

Copy link
Contributor

@sarah-witt sarah-witt left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@sarah-witt sarah-witt merged commit 9c5ac5e into DataDog:master Jun 6, 2022
github-actions bot pushed a commit that referenced this pull request Jun 6, 2022
* Update README.md

Adjusting airflow matching pattern to include missing `airflow.`, as well as unifying tags for dag and task id (dag_id / task_id) as they were different between mapping

* Update README.md

* Update README.md

* Update datadog_values.yaml

* Update test_check_metrics_up_to_date.py 9c5ac5e
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.

None yet

6 participants