Skip to content

Conversation

@coinzerge
Copy link

@coinzerge coinzerge commented Sep 30, 2021

closes: #18599


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Sep 30, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 30, 2021

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@coinzerge coinzerge changed the title use seconds Use seconds for schedule_delay metric Sep 30, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Sep 30, 2021
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

This belongs in the DataDogstatsd adapter inside airflow.stats -- as the normal statsd client accepts timedelta.

I.e. add an isinstance check in here

airflow/airflow/stats.py

Lines 333 to 339 in abdede8

@validate_stat
def timing(self, stat, dt, tags=None):
"""Stats timing"""
if self.allow_list_validator.test(stat):
tags = tags or []
return self.dogstatsd.timing(metric=stat, value=dt, tags=tags)
return None

@coinzerge coinzerge requested review from ashb and potiuk September 30, 2021 23:23
@uranusjr
Copy link
Member

uranusjr commented Oct 3, 2021

Agreed, the stat backend should be able to handle timedelta instead.

@coinzerge
Copy link
Author

coinzerge commented Oct 6, 2021

@ashb @uranusjr updated PR

@uranusjr
Copy link
Member

uranusjr commented Oct 6, 2021

Could you add a test for this in tests/core/test_stats.py? And while we're at it, we should also add annotations to dt in the stats module to make it explicit it can be either float or timedelta.

@coinzerge
Copy link
Author

added test. what do you mean by we should also add annotations?

@uranusjr
Copy link
Member

uranusjr commented Oct 7, 2021

Function parameter annotations. The functions are already partially annotated, but we should annotate dt with Union[float, datetime.timedelta].

@coinzerge
Copy link
Author

@uranusjr anything else i need to do to merge the PR?

@uranusjr
Copy link
Member

uranusjr commented Oct 8, 2021

You need to first fix the static checks and tests. Looks like your Union import is breaking things.

@coinzerge coinzerge requested a review from uranusjr October 9, 2021 02:33
@potiuk
Copy link
Member

potiuk commented Oct 9, 2021

Now tests are faling

@ashb
Copy link
Member

ashb commented Oct 12, 2021

I'm still not 100% sure this is "right" as many of the timer metrics we track are <1s, so this would just submit 0 values for them making them useless.

ashb
ashb previously requested changes Oct 12, 2021
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

I've just double checked and no, this is not right.

Using the code example from https://docs.datadoghq.com/metrics/dogstatsd_metrics_submission/#timer

from datadog import initialize, statsd
import time
import random

options = {
    'statsd_host':'127.0.0.1',
    'statsd_port':8125
}

initialize(**options)

@statsd.timed('example_metric.timer', tags=["environment:dev,function:my_function"])
def my_function():
  time.sleep(random.randint(0, 10))

my_function()

this is what I see submited to statsd:

example_metric.timer:1.836400042520836e-05|ms|#environment:dev_function:my_function

@coinzerge
Copy link
Author

what's not right? total_seconds returns a float.

@ashb
Copy link
Member

ashb commented Oct 13, 2021

what's not right? total_seconds returns a float.

🤦🏻 Of course it does. I thought it was integer seconds only. Sorry

@ashb ashb dismissed their stale review October 13, 2021 11:57

I can't python

@coinzerge coinzerge requested a review from ashb October 15, 2021 05:25
@potiuk potiuk added the telemetry Telemetry-related issues label Oct 26, 2021
@kaxil
Copy link
Member

kaxil commented Nov 12, 2021

Tests are failing, can you take a look plz

@eladkal
Copy link
Contributor

eladkal commented Nov 17, 2021

@coinzerge can you please fix the test failure?
tests/core/test_stats.py::TestDogStats::test_timing: AssertionError: Expected 'timing' to be called once. Called 2 times.

@ashb
Copy link
Member

ashb commented Dec 15, 2021

Fixed by #19973

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge telemetry Telemetry-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

datadog parsing error for dagrun.schedule_delay since it is not passed in float type

7 participants