feat(profiling): add serverless scheduler and conditionally use it #3870

astuyve · 2022-06-27T19:09:00Z

feat: WIP profiling support for Python
wip: function name as tag
feat: custom serverless scheduler to wake up every second and check if we've acquired 60s of data before flushing
feat: Conditionally use serverless scheduler. Refactor to inherit from base scheduler and override periodic method
fix: use 60s, not 61s
feat: test. Back to 60s
feat: Fix double flushing issue by comparing last export time as well as counting number of profiled seconds
feat: riot formatter
fix: _interval should be int, not float
feat: remove unneeded .encode()
feat: revert setup
feat: Move encode to after nil check
feat: Guard against unset _last_export. Add tests for overriden periodic function.
feat: formatting

Description

Based on Julien's feedback in our RFC, I've added a small scheduler which flushes after the periodic function is called 60 times. This ensures that profiles are sampled over the course of 60s of runtime instead of wallclock time.

Checklist

Title must conform to conventional commit.
Add additional sections for feat and fix pull requests.
Ensure tests are passing for affected code.
Library documentation and/or Datadog's documentation site is updated. Link to doc PR in description.

Motivation

Design

Testing strategy

Relevant issue(s)

Testing strategy

Reviewer Checklist

codecov-commenter · 2022-07-01T13:16:12Z

Codecov Report

Merging #3870 (fc2c22b) into 1.x (e618c0f) will decrease coverage by 0.79%.
The diff coverage is 43.82%.

@@            Coverage Diff             @@
##              1.x    #3870      +/-   ##
==========================================
- Coverage   79.86%   79.06%   -0.80%     
==========================================
  Files         717      719       +2     
  Lines       56347    56898     +551     
==========================================
- Hits        45004    44989      -15     
- Misses      11343    11909     +566

Impacted Files	Coverage Δ
ddtrace/contrib/flask/patch.py	`0.00% <0.00%> (ø)`
ddtrace/profiling/profiler.py	`0.00% <0.00%> (ø)`
ddtrace/profiling/scheduler.py	`0.00% <0.00%> (ø)`
tests/contrib/django/django_app/settings.py	`100.00% <ø> (ø)`
tests/contrib/django/test_django.py	`100.00% <ø> (ø)`
tests/contrib/flask/test_flask_appsec.py	`0.00% <0.00%> (ø)`
tests/contrib/flask/test_flask_snapshot.py	`0.00% <0.00%> (ø)`
tests/contrib/flask/test_hooks.py	`0.00% <0.00%> (ø)`
tests/contrib/flask/test_request.py	`0.00% <0.00%> (ø)`
tests/contrib/flask/test_static.py	`0.00% <0.00%> (ø)`
... and 38 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ddtrace/profiling/exporter/http.py

ddtrace/profiling/profiler.py

ddtrace/profiling/serverless_scheduler.py

releasenotes/notes/feat-serverless-scheduler-31a819bc9eb4f332.yaml

jd

I'm a bit confused on how this solves the problem. You write:

Based on Julien's feedback in our RFC, I've added a small scheduler which flushes after the periodic function is called 60 times. This ensures that profiles are sampled over the course of 60s of runtime instead of wallclock time.

You're still doing 60 × 1s which is 60s of runtime. It's just that this now sleeps 60 times 1 second, rather than 1 time 60 seconds.

How is that going to solve the "serverless" problem? (if that solves it we should write it in the commit/ PR body)

ddtrace/profiling/profiler.py

ddtrace/profiling/serverless_scheduler.py

jd

a few changes but LGTM otherwise

jd · 2022-08-04T10:13:56Z

ddtrace/profiling/profiler.py

+    _scheduler = attr.ib(
+        init=False, default=None, type=Union[scheduler.Scheduler, serverless_scheduler.ServerlessScheduler]
+    )


Suggested change

_scheduler = attr.ib(

init=False, default=None, type=Union[scheduler.Scheduler, serverless_scheduler.ServerlessScheduler]

)

_scheduler = attr.ib(

init=False, default=None, type=scheduler.Scheduler,

)

The serverless scheduler is a subclass so that's ok

I've tried this, when combined with the comment to remove the Union at line 225, mypy still fails:

ddtrace/profiling/profiler.py:222: error: Incompatible types in assignment (expression has type "Type[Scheduler]", variable has type "Type[ServerlessScheduler]") [assignment] Found 1 error in 1 file (checked 404 source files) Test failed with exit code 1

Can you show line 225 when you have this error?
It seems that you are assigning a class rather than instance on L225.

jd · 2022-08-04T10:15:07Z

ddtrace/profiling/profiler.py

+            if self._lambda_function_name:
+                scheduler_class = (
+                    serverless_scheduler.ServerlessScheduler
+                )  # type: Union[type[scheduler.Scheduler], type[serverless_scheduler.ServerlessScheduler]]


I don't think you need type then here
otherwise typing.Type[serverless_scheduler.ServerlessScheduler] or typing.Type[sscheduler.Scheduler] should be what you want I guess.

See above comment, this fails mypy

ddtrace/profiling/serverless_scheduler.py

…f we've acquired 60s of data before flushing

…m base scheduler and override periodic method

… as counting number of profiled seconds

…dic function.

…uce to once every minute. Test changes to match

Kyle-Verhoog

just a few nits

ddtrace/profiling/profiler.py

…ing utf-8 encoding explicitly

Kyle-Verhoog · 2022-08-08T15:56:22Z

@Mergifyio backport 1.4

mergify · 2022-08-08T15:56:29Z

backport 1.4

✅ Backports have been created

#4058 feat(profiling): add serverless scheduler and conditionally use it (backport #3870) has been created for branch 1.4

…3870) Based on Julien's feedback in our RFC, I've added a small scheduler which flushes after the periodic function is called 60 times. This ensures that profiles are sampled over the course of 60s of runtime instead of wallclock time. (cherry picked from commit 83ce6ce)

…3870) (#4058) Based on Julien's feedback in our RFC, I've added a small scheduler which flushes after the periodic function is called 60 times. This ensures that profiles are sampled over the course of 60s of runtime instead of wallclock time. (cherry picked from commit 83ce6ce) Co-authored-by: AJ Stuyvenberg <[email protected]>

astuyve requested a review from a team as a code owner June 27, 2022 19:09

astuyve changed the title ~~feat(profiling): Add serverless scheduler and conditionally use it.~~ feat(profiling): add serverless scheduler and conditionally use it Jun 27, 2022

astuyve requested a review from jd June 27, 2022 19:18

jd suggested changes Jul 4, 2022

View reviewed changes

astuyve requested a review from jd July 18, 2022 15:46

jd suggested changes Jul 26, 2022

View reviewed changes

jd suggested changes Aug 4, 2022

View reviewed changes

astuyve added 22 commits August 5, 2022 14:35

feat: WIP profiling support for Python

90df359

wip: function name as tag

0f3fd52

feat: custom serverless scheduler to wake up every second and check i…

c06cd4f

…f we've acquired 60s of data before flushing

feat: Conditionally use serverless scheduler. Refactor to inherit fro…

5c9ec7b

…m base scheduler and override periodic method

fix: use 60s, not 61s

cf15cd0

feat: test. Back to 60s

6191541

feat: Fix double flushing issue by comparing last export time as well…

80c7133

… as counting number of profiled seconds

feat: riot formatter

9329c26

fix: _interval should be int, not float

5ecd1a2

feat: remove unneeded .encode()

d104d70

feat: revert setup

cf376df

feat: Move encode to after nil check

0978935

feat: Guard against unset _last_export. Add tests for overriden perio…

b8c4460

…dic function.

feat: formatting

4afe52c

feat: release notes

754367c

feat: Add serverless to spelling list

d3e5a4c

feat: Incorporate feedback from JD

79c2f23

feat: CR feedback from JD

94cd7f2

lint: fix

3e5c9e4

feat: remove unused import

811cb04

feat: Human-readable release note

1ab1ef3

feat: CR feedback to remove unneeded checks on periodic calls and red…

baadc76

…uce to once every minute. Test changes to match

jd force-pushed the aj/serverless-profiling branch from 7611d12 to e2205ab Compare August 5, 2022 12:55

try fixing typing

e600cbe

jd force-pushed the aj/serverless-profiling branch from e2205ab to e600cbe Compare August 5, 2022 12:55

jd added 2 commits August 5, 2022 15:09

move serverless into scheduler

662f681

use some var

528e325

jd force-pushed the aj/serverless-profiling branch from d6c96f1 to 528e325 Compare August 5, 2022 13:52

jd added 2 commits August 5, 2022 16:08

test(serverless): add profiler test

6e2ab1d

fix profile test case

57eef9c

jd previously approved these changes Aug 5, 2022

View reviewed changes

mabdinur previously approved these changes Aug 5, 2022

View reviewed changes

Kyle-Verhoog reviewed Aug 5, 2022

View reviewed changes

ddtrace/profiling/profiler.py Outdated Show resolved Hide resolved

ddtrace/profiling/profiler.py Outdated Show resolved Hide resolved

ddtrace/profiling/profiler.py Outdated Show resolved Hide resolved

mergify bot and others added 2 commits August 5, 2022 20:28

Merge branch '1.x' into aj/serverless-profiling

e07955f

feat: more CR feedback. Flipping implicit none checks to explicit, us…

fc2c22b

…ing utf-8 encoding explicitly

astuyve dismissed stale reviews from mabdinur and jd via fc2c22b August 8, 2022 10:56

astuyve requested review from mabdinur, jd and Kyle-Verhoog August 8, 2022 10:56

Merge branch '1.x' into aj/serverless-profiling

3cbc43b

Kyle-Verhoog approved these changes Aug 8, 2022

View reviewed changes

Kyle-Verhoog added this to the v1.4.0 milestone Aug 8, 2022

Kyle-Verhoog merged commit 83ce6ce into 1.x Aug 8, 2022

Kyle-Verhoog deleted the aj/serverless-profiling branch August 8, 2022 15:57

mergify bot mentioned this pull request Aug 8, 2022

feat(profiling): add serverless scheduler and conditionally use it (backport #3870) #4058

Merged

github-actions bot modified the milestones: v1.4.0, v1.5.0 Aug 8, 2022

feat(profiling): add serverless scheduler and conditionally use it #3870

feat(profiling): add serverless scheduler and conditionally use it #3870

Uh oh!

Conversation

astuyve commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation

Design

Testing strategy

Relevant issue(s)

Testing strategy

Reviewer Checklist

Uh oh!

codecov-commenter commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jd left a comment

Choose a reason for hiding this comment

Uh oh!

jd Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

astuyve Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

jd Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

jd Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

astuyve Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kyle-Verhoog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kyle-Verhoog commented Aug 8, 2022

Uh oh!

mergify bot commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Uh oh!

astuyve commented Jun 27, 2022 •

edited

Loading

codecov-commenter commented Jul 1, 2022 •

edited

Loading

mergify bot commented Aug 8, 2022 •

edited

Loading