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

Refactor SafeDogStatsdLogger to use get_validator to enable pattern matching #39370

Merged
merged 5 commits into from
May 6, 2024

Conversation

rawwar
Copy link
Contributor

@rawwar rawwar commented May 2, 2024

closes #39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.

@rawwar rawwar changed the title [feat] Add pattern matching to SafeDogStatsdLogger [feat] Refactor SafeDogStatsdLogger to use get_validator to get validators May 2, 2024
@rawwar rawwar marked this pull request as ready for review May 2, 2024 13:25
@rawwar rawwar changed the title [feat] Refactor SafeDogStatsdLogger to use get_validator to get validators Refactor SafeDogStatsdLogger to use get_validator to get validators May 2, 2024
@rawwar rawwar changed the title Refactor SafeDogStatsdLogger to use get_validator to get validators Refactor SafeDogStatsdLogger to use get_validator to enable pattern matching May 2, 2024
Copy link
Member

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

were you able to test this end to end with Datadog using all the 3 configurations? Allow list, block list and pattern list? Also could you please add tests for all those configurations if they don't exist already?

@rawwar
Copy link
Contributor Author

rawwar commented May 3, 2024

@pankajkoti, Yes, I have tested them. Allow/Block list without pattern worked perfectly. However, I noticed the following behaviour when using pattern matching, which I am not sure if it's right.

With pattern matching enabled:

  1. AIRFLOW__METRICS__METRICS_ALLOW_LIST = .*queued_duration.* : Worked perfectly
image
  1. AIRFLOW__METRICS__METRICS_ALLOW_LIST = .*get_astronauts.* : Not sure if this is correct
image

In the 2nd screenshot, I still see several metrics that don't have get_astronauts anywhere in their metrics names. Is this correct behaviour? One of them is airflow.ti.finish

@rawwar
Copy link
Contributor Author

rawwar commented May 3, 2024

Block lists seem to be working as intended.

  1. AIRFLOW__METRICS__METRICS_BLOCK_LIST = .*duration.* : works perfectly
image

FYI: I have been using different tags for different settings to differentiate between searches.

@rawwar
Copy link
Contributor Author

rawwar commented May 3, 2024

Env variables set:

AIRFLOW__METRICS__STATSD_DATADOG_ENABLED true
AIRFLOW__METRICS__STATSD_DATADOG_TAGS testNo:C2
DATADOG_API_KEY ● ● ● ● ● ● ●
AIRFLOW__METRICS__METRICS_USE_PATTERN_MATCH true
AIRFLOW__METRICS__METRICS_ALLOW_LIST .*print_astronaut_craft.*

@pankajkoti pankajkoti requested a review from ferruzzi May 3, 2024 11:59
Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

Code change LGTM, but I'm not sure that second allow-list example (the one you seemed unsure about) is working as expected. Either way, that is not in scope for this PR if that is broken, but you may want to look into that more if you have the time.

@rawwar
Copy link
Contributor Author

rawwar commented May 4, 2024

Code change LGTM, but I'm not sure that second allow-list example (the one you seemed unsure about) is working as expected. Either way, that is not in scope for this PR if that is broken, but you may want to look into that more if you have the time.

I will look into it. I will create an issue( #39400) and post my findings there.

@pankajkoti pankajkoti merged commit 0c4ffb8 into apache:main May 6, 2024
41 checks passed
@rawwar rawwar deleted the kalyan/datadog_add_pattern_match branch May 6, 2024 11:34
@pankajkoti pankajkoti added this to the Airflow 2.9.2 milestone May 6, 2024
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label May 7, 2024
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
…rn matching (apache#39370)

closes apache#39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.
pateash pushed a commit to pateash/airflow that referenced this pull request May 13, 2024
…rn matching (apache#39370)

closes apache#39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.
ephraimbuddy pushed a commit that referenced this pull request Jun 4, 2024
…rn matching (#39370)

closes #39368

By reusing the get_validators function, we can incorporate pattern search to include allow/block lists.

(cherry picked from commit 0c4ffb8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a pattern-matching support for allow and block list in Datadog metric
5 participants