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

chore(iast): remote config thread workaround for gevent #4749

Conversation

gnufede
Copy link
Member

@gnufede gnufede commented Dec 7, 2022

Description

Workaround for remote config to work properly with gevent.

This PR ensures the Remote config worker thread is launched at the very end, not interrupting the gevent patching mechanism.

Without this PR, the gevent patching is interrupted so if gevent is used, it won't work as expected.

As a side effect, if gevent is used and Django is installed, our Django patching mechanism collides with gevent raising a Django ImproperlyConfigured error.

Checklist

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added and follows the library release note guidelines, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.

@gnufede gnufede added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring labels Dec 7, 2022
@gnufede gnufede marked this pull request as ready for review December 9, 2022 15:23
@gnufede gnufede requested review from a team as code owners December 9, 2022 15:23
ddtrace/tracer.py Outdated Show resolved Hide resolved
@gnufede gnufede force-pushed the gnufede/APPSEC-6558-start-rcm-thread-when-writer-thread-starts-workaround-wip branch from 3466dcf to d64eec9 Compare December 14, 2022 12:02
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2022

Codecov Report

Merging #4749 (e1b55aa) into 1.x (ee6938f) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##              1.x    #4749   +/-   ##
=======================================
  Coverage   74.55%   74.55%           
=======================================
  Files         812      812           
  Lines       62131    62131           
=======================================
  Hits        46324    46324           
  Misses      15807    15807           
Impacted Files Coverage Δ
ddtrace/tracer.py 61.00% <ø> (+0.21%) ⬆️
ddtrace/appsec/_remoteconfiguration.py 47.82% <50.00%> (+2.37%) ⬆️
ddtrace/internal/writer.py 56.42% <50.00%> (-0.05%) ⬇️
tests/utils.py 90.18% <0.00%> (-0.24%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ddtrace/internal/writer.py Outdated Show resolved Hide resolved
ddtrace/tracer.py Outdated Show resolved Hide resolved
ddtrace/tracer.py Show resolved Hide resolved
@gnufede gnufede force-pushed the gnufede/APPSEC-6558-start-rcm-thread-when-writer-thread-starts-workaround-wip branch 4 times, most recently from ea10d46 to a12984a Compare December 20, 2022 12:07
@gnufede gnufede requested a review from a team as a code owner December 20, 2022 12:07
@gnufede gnufede force-pushed the gnufede/APPSEC-6558-start-rcm-thread-when-writer-thread-starts-workaround-wip branch from a12984a to f732779 Compare December 20, 2022 12:08
juanjux
juanjux previously approved these changes Dec 20, 2022
P403n1x87
P403n1x87 previously approved these changes Dec 20, 2022
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

nit, otherwise lgtm

ddtrace/appsec/_remoteconfiguration.py Show resolved Hide resolved
@gnufede gnufede enabled auto-merge (squash) December 20, 2022 19:08
@gnufede gnufede merged commit 0dfd930 into 1.x Dec 21, 2022
@gnufede gnufede deleted the gnufede/APPSEC-6558-start-rcm-thread-when-writer-thread-starts-workaround-wip branch December 21, 2022 07:55
@github-actions github-actions bot added this to the v1.8.0 milestone Dec 21, 2022
gnufede added a commit that referenced this pull request Dec 22, 2022
## Description
Workaround for remote config to work properly with gevent.

This PR ensures the Remote config worker thread is launched at the very
end, not interrupting the gevent patching mechanism.

Without this PR, the gevent patching is interrupted so if gevent is
used, it won't work as expected.

As a side effect, if gevent is used and Django is installed, our Django
patching mechanism collides with gevent raising a Django
ImproperlyConfigured error.

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.


## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
gnufede added a commit that referenced this pull request Dec 22, 2022
## Description
Workaround for remote config to work properly with gevent.

This PR ensures the Remote config worker thread is launched at the very
end, not interrupting the gevent patching mechanism.

Without this PR, the gevent patching is interrupted so if gevent is
used, it won't work as expected.

As a side effect, if gevent is used and Django is installed, our Django
patching mechanism collides with gevent raising a Django
ImproperlyConfigured error.

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.


## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
mabdinur pushed a commit that referenced this pull request Dec 23, 2022
… to 1.7] (#4824)

## Description

Workaround for remote config to work properly with gevent.

This PR ensures the Remote config worker thread is launched at the very
end, not interrupting the gevent patching mechanism.

Without this PR, the gevent patching is interrupted so if gevent is
used, it won't work as expected.

As a side effect, if gevent is used and Django is installed, our Django
patching mechanism collides with gevent raising a Django
ImproperlyConfigured error.

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.


## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants