[AIRFLOW-7008] Add perf kit with common used decorators/contexts #7650

mik-laj · 2020-03-07T21:30:46Z

Thanks for support to @evgenyshulman from Databand!

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.
Read the Pull Request Guidelines for more information.

potiuk

Fantastic @mik-laj ! I love it. I think there should be some short document describing how to use those. But I am ok with merging it as it is and adding a document later.

potiuk · 2020-03-08T03:29:57Z

Some static checks need to be fixed as well :)

mik-laj · 2020-03-08T03:39:51Z

I want to add documentation in this PR, but for now I wanted to take the first step. This PR is still WIP.

ashb

With the sql alchemy bit here should we remove/update the previous query trace scripts?

mik-laj · 2020-03-08T18:34:19Z

Previous scripts track SQL queries for many processes and for all code. This script allows you to trace only part of the code and doesn't work properly with many processes. We also cannot share the code, because perf_kit is a separate package to be able to monitor the Airflow initialization process.

scripts/perf/perf_kit/imports.py

scripts/perf/perf_kit/retry_and_time.py

codecov-io · 2020-03-09T21:03:48Z

Codecov Report

Merging #7650 into master will decrease coverage by 0.76%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7650      +/-   ##
==========================================
- Coverage   88.45%   87.68%   -0.77%     
==========================================
  Files         937      940       +3     
  Lines       45234    45355     +121     
==========================================
- Hits        40011    39770     -241     
- Misses       5223     5585     +362

Impacted Files	Coverage Δ
...flow/providers/apache/cassandra/hooks/cassandra.py	`21.25% <0.00%> (-72.50%)`	⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py	`35.84% <0.00%> (-64.16%)`	⬇️
airflow/kubernetes/volume_mount.py	`44.44% <0.00%> (-55.56%)`	⬇️
airflow/providers/postgres/operators/postgres.py	`47.82% <0.00%> (-52.18%)`	⬇️
airflow/providers/redis/operators/redis_publish.py	`50.00% <0.00%> (-50.00%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0.00%> (-47.06%)`	⬇️
airflow/providers/mongo/sensors/mongo.py	`53.33% <0.00%> (-46.67%)`	⬇️
airflow/kubernetes/pod_launcher.py	`47.18% <0.00%> (-45.08%)`	⬇️
airflow/providers/mysql/operators/mysql.py	`55.00% <0.00%> (-45.00%)`	⬇️
airflow/providers/redis/sensors/redis_key.py	`61.53% <0.00%> (-38.47%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45c8983...82205bc. Read the comment docs.

mik-laj · 2020-04-14T21:09:21Z

I added documentation. Does someone want to look at these changes? That would be helpful.
CC: @zhongjiajie

zhongjiajie · 2020-04-15T01:53:28Z

Should we add some document to CONTRIBUTING.rst too?

mik-laj · 2020-04-15T02:15:43Z

@zhongjiajie I am afraid that this file will be too long if we add any information. In my opinion, the CONTRIBUTOR Guide should contain the most important information for beginners. Scheduler optimization is not a key task for these people. I would like to see a guidebook that describes more advanced information on the model of a contributor's guide, but advanced people are already quite familiar with the structure of the project. WDYT?

scripts/perf/perf_kit/__init__.py

zhongjiajie · 2020-04-15T02:28:30Z

In my opinion, the CONTRIBUTOR Guide should contain the most important information for beginners.

Make sence, but I still think we should add doc somewhere else, not only in python docsting

zhongjiajie · 2020-04-15T02:33:14Z

BTW, I find out we have some scheduler doc only in confluence https://cwiki.apache.org/confluence/display/AIRFLOW/Scheduler+Basics, should we add this page to airflow document too

mik-laj · 2020-04-15T02:34:59Z

I will add a short note in the file TESTING.rst.

mik-laj · 2020-04-15T02:51:13Z

I saw this article, but I'm afraid we should check and update it first. I started to write internal materials for my company. I started to write internal materials for my company. I hope that some excerpts can be published on the company blog sometime.

zhongjiajie · 2020-04-15T03:28:29Z

Looking forward your public blog, BTW, code&doc LGTM, except TESTING.rst what you mention above

scripts/perf/perf_kit/__init__.py

scripts/perf/perf_kit/memory.py

ashb · 2020-04-15T09:37:07Z

scripts/perf/perf_kit/repeat_and_time.py

+    # Example 2:
+    REPEAT_COUNT = 5
+
+    @timing(REPEAT_COUNT)


Suggested change

@timing(REPEAT_COUNT)

Timing is here twice - don't think you want this one?

Yes. In this case, the average for all runs will be calculated.

kaxil · 2020-04-15T13:03:08Z

scripts/perf/perf_kit/__init__.py

+Personally, I also have a separate file - a notebook in which I save various test cases and run them
+like a classic Python program.


Let's use the third person here or something like "Having a separate file to save various tests cases can be helpful"

scripts/perf/perf_kit/__init__.py

Co-Authored-By: Kaxil Naik <kaxilnaik@gmail.com>

scripts/perf/perf_kit/__init__.py

scripts/perf/perf_kit/python.py

kaxil · 2020-04-21T11:16:10Z

scripts/perf/perf_kit/sqlalchemy.py

+
+
+@contextlib.contextmanager
+def trace_queries(display_time=True, display_trace=True, display_sql=False, displaay_parameters=True):


Suggested change

def trace_queries(display_time=True, display_trace=True, display_sql=False, displaay_parameters=True):

def trace_queries(display_time=True, display_trace=True, display_sql=False, display_parameters=True):

kaxil · 2020-04-21T11:16:18Z

scripts/perf/perf_kit/sqlalchemy.py

+    :param display_time: If True, displays the query execution time.
+    :param display_trace: If True, displays the simplified (one-line) stack trace
+    :param display_sql: If True, displays the SQL statements
+    :param displaay_parameters: If True, display SQL statement parameters


Suggested change

:param displaay_parameters: If True, display SQL statement parameters

:param display_parameters: If True, display SQL statement parameters

kaxil

Looks good, just few minor doc related changes suggested

Co-Authored-By: Kaxil Naik <kaxilnaik@gmail.com>

milton0825 · 2020-04-22T04:03:11Z

scripts/perf/perf_kit/python.py

+    Running py-spy inside of a docker container will also usually bring up a permissions denied error
+    even when running as root.
+
+    This error is caused by docker restricting the process_vm_readv system call we are using. This can be


Is it possible to surface the solution as part of the error message to user in case we run into the permission error?

It will be difficult because this error happens in the subprocess. It would be necessary to check whether this process started correctly, which will affect the result. When we run the process, we immediately need to start executing the observed code. Otherwise, we will have junk data in the diagram.

boring-cyborg bot added the area:dev-tools label Mar 7, 2020

mik-laj force-pushed the AIRFLOW-7008 branch from c41aa5c to 49c22a3 Compare March 7, 2020 21:31

mik-laj added the area:performance label Mar 7, 2020

mik-laj force-pushed the AIRFLOW-7008 branch 2 times, most recently from 5935904 to 58b3822 Compare March 7, 2020 21:47

potiuk approved these changes Mar 8, 2020

View reviewed changes

mik-laj requested a review from potiuk March 8, 2020 03:38

ashb reviewed Mar 8, 2020

View reviewed changes

ashb reviewed Mar 9, 2020

View reviewed changes

scripts/perf/perf_kit/imports.py Outdated Show resolved Hide resolved

ashb reviewed Mar 9, 2020

View reviewed changes

scripts/perf/perf_kit/retry_and_time.py Outdated Show resolved Hide resolved

ashb reviewed Mar 9, 2020

View reviewed changes

scripts/perf/perf_kit/retry_and_time.py Outdated Show resolved Hide resolved

ashb reviewed Mar 9, 2020

View reviewed changes

scripts/perf/perf_kit/retry_and_time.py Outdated Show resolved Hide resolved

mik-laj force-pushed the AIRFLOW-7008 branch from 626ddea to 44e026d Compare March 18, 2020 13:23

mik-laj mentioned this pull request Apr 12, 2020

Dag bulk_sync_to_db dag_tag only remove not exists #8231

Merged

5 tasks

[AIRFLOW-7008] Add perf kit with common used decorators/contexts

fd5ad7e

mik-laj force-pushed the AIRFLOW-7008 branch from f7bcb62 to fd5ad7e Compare April 14, 2020 15:40

mik-laj changed the title ~~[AIRFLOW-7008][WIP] Add perf kit with common used decorators/contexts~~ [AIRFLOW-7008] Add perf kit with common used decorators/contexts Apr 14, 2020

fixup! [AIRFLOW-7008] Add perf kit with common used decorators/contexts

82205bc

zhongjiajie reviewed Apr 15, 2020

View reviewed changes

scripts/perf/perf_kit/__init__.py Show resolved Hide resolved

milton0825 reviewed Apr 15, 2020

View reviewed changes

scripts/perf/perf_kit/__init__.py Outdated Show resolved Hide resolved

ashb reviewed Apr 15, 2020

View reviewed changes

scripts/perf/perf_kit/memory.py Show resolved Hide resolved

ashb reviewed Apr 15, 2020

View reviewed changes

kaxil reviewed Apr 15, 2020

View reviewed changes

Kamil Breguła added 3 commits April 20, 2020 02:38

Fix path - script => scripts, add :emphasize-lines

6af75e4

Fix docs - use third-person

7a86711

Add gc_collect parameter

e6a112b

kaxil reviewed Apr 20, 2020

View reviewed changes

scripts/perf/perf_kit/__init__.py Outdated Show resolved Hide resolved

Update scripts/perf/perf_kit/__init__.py

82918fb

Co-Authored-By: Kaxil Naik <kaxilnaik@gmail.com>

kaxil reviewed Apr 21, 2020

View reviewed changes

scripts/perf/perf_kit/__init__.py Outdated Show resolved Hide resolved

kaxil reviewed Apr 21, 2020

View reviewed changes

scripts/perf/perf_kit/__init__.py Outdated Show resolved Hide resolved

kaxil reviewed Apr 21, 2020

View reviewed changes

scripts/perf/perf_kit/python.py Outdated Show resolved Hide resolved

kaxil reviewed Apr 21, 2020

View reviewed changes

mik-laj and others added 2 commits April 22, 2020 05:06

Apply suggestions from code review

ddc10de

Co-Authored-By: Kaxil Naik <kaxilnaik@gmail.com>

fixup! Apply suggestions from code review

305f8f1

milton0825 reviewed Apr 22, 2020

View reviewed changes

milton0825 approved these changes Apr 22, 2020

View reviewed changes

kaxil approved these changes Apr 22, 2020

View reviewed changes

mik-laj merged commit aec768b into apache:master May 4, 2020

mik-laj deleted the AIRFLOW-7008 branch May 4, 2020 01:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-7008] Add perf kit with common used decorators/contexts #7650

[AIRFLOW-7008] Add perf kit with common used decorators/contexts #7650

mik-laj commented Mar 7, 2020 •

edited

Loading

potiuk left a comment

potiuk commented Mar 8, 2020

mik-laj commented Mar 8, 2020 •

edited

Loading

ashb left a comment

mik-laj commented Mar 8, 2020

codecov-io commented Mar 9, 2020 •

edited

Loading

mik-laj commented Apr 14, 2020 •

edited

Loading

zhongjiajie commented Apr 15, 2020

mik-laj commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020

mik-laj commented Apr 15, 2020

mik-laj commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020 •

edited

Loading

ashb Apr 15, 2020

mik-laj Apr 15, 2020

kaxil Apr 15, 2020

kaxil Apr 21, 2020

kaxil Apr 21, 2020

kaxil left a comment

milton0825 Apr 22, 2020

mik-laj May 4, 2020

		Personally, I also have a separate file - a notebook in which I save various test cases and run them
		like a classic Python program.



		@contextlib.contextmanager
		def trace_queries(display_time=True, display_trace=True, display_sql=False, displaay_parameters=True):

	:param displaay_parameters: If True, display SQL statement parameters
	:param display_parameters: If True, display SQL statement parameters

[AIRFLOW-7008] Add perf kit with common used decorators/contexts #7650

[AIRFLOW-7008] Add perf kit with common used decorators/contexts #7650

Conversation

mik-laj commented Mar 7, 2020 • edited Loading

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Mar 8, 2020

mik-laj commented Mar 8, 2020 • edited Loading

ashb left a comment

Choose a reason for hiding this comment

mik-laj commented Mar 8, 2020

codecov-io commented Mar 9, 2020 • edited Loading

Codecov Report

mik-laj commented Apr 14, 2020 • edited Loading

zhongjiajie commented Apr 15, 2020

mik-laj commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020

mik-laj commented Apr 15, 2020

mik-laj commented Apr 15, 2020

zhongjiajie commented Apr 15, 2020 • edited Loading

ashb Apr 15, 2020

Choose a reason for hiding this comment

mik-laj Apr 15, 2020

Choose a reason for hiding this comment

kaxil Apr 15, 2020

Choose a reason for hiding this comment

kaxil Apr 21, 2020

Choose a reason for hiding this comment

kaxil Apr 21, 2020

Choose a reason for hiding this comment

kaxil left a comment

Choose a reason for hiding this comment

milton0825 Apr 22, 2020

Choose a reason for hiding this comment

mik-laj May 4, 2020

Choose a reason for hiding this comment

mik-laj commented Mar 7, 2020 •

edited

Loading

mik-laj commented Mar 8, 2020 •

edited

Loading

codecov-io commented Mar 9, 2020 •

edited

Loading

mik-laj commented Apr 14, 2020 •

edited

Loading

zhongjiajie commented Apr 15, 2020 •

edited

Loading