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

feat(starlette): add resource aggregation #1714

Merged
merged 47 commits into from
Oct 19, 2020

Conversation

Isabella-Pham
Copy link
Contributor

@Isabella-Pham Isabella-Pham commented Oct 7, 2020

Description

The current Starlette integration uses ASGI middleware and does not have Starlette specific features. This change adds Starlette specific features to the Starlette integration.

Checklist

  • Aggregate resource statistics by default
  • Allow option to turn resource aggregation off by setting config.starlette["aggregate_resources"] to False

majorgreys and others added 30 commits September 15, 2020 16:06
…t for starlette by passing the starlette config
* added starlette to circleci config
* removed starlette utils as it is not needed
* checked for sleep parameter in starlette test app.py
* removed starlette 12 from tox.ini
…DataDog/dd-trace-py into Isabella-Pham/starlette-auto-middleware

.
* modified config variable name to integration_config in the asgi middleware
…DataDog/dd-trace-py into Isabella-Pham/starlette-auto-middleware

updating branch
…DataDog/dd-trace-py into Isabella-Pham/starlette-auto-middleware

Updating branch
* modified documentation in __init__.py to include "aggregate_resource"
* asgi middleware checks for "aggregate_resource" in config and will aggregate resource if True
* added tests to check that resources are properly aggregated upon request
…ggregate resources in the config (no need to pass anything into patch()

* added more tests
…patch

looking into changing the current approach from modifying the scope to modifying the span
@majorgreys majorgreys added this to the 0.44.0 milestone Oct 15, 2020
@Isabella-Pham Isabella-Pham changed the title feat(starlette): add starlette specific features to integration feat(starlette): add resource aggregation Oct 15, 2020
@Isabella-Pham Isabella-Pham marked this pull request as ready for review October 15, 2020 16:16
@Isabella-Pham Isabella-Pham requested a review from a team as a code owner October 15, 2020 16:16
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👏 nice job! just a few things to address. I really like the clean interaction between the integration and the asgi middleware -- it sets a good precedent for future asgi web framework integrations.

ddtrace/contrib/asgi/middleware.py Outdated Show resolved Hide resolved
ddtrace/contrib/starlette/patch.py Outdated Show resolved Hide resolved
tests/contrib/starlette/test_starlette.py Show resolved Hide resolved
tests/contrib/starlette/test_starlette.py Show resolved Hide resolved
ddtrace/contrib/starlette/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/starlette/patch.py Outdated Show resolved Hide resolved
ddtrace/contrib/starlette/patch.py Outdated Show resolved Hide resolved
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Oct 16, 2020
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👍 great stuff!

majorgreys
majorgreys previously approved these changes Oct 16, 2020
@majorgreys majorgreys dismissed stale reviews from Kyle-Verhoog and themself via 90ecb16 October 19, 2020 14:40
@majorgreys majorgreys force-pushed the Isabella-Pham/starlette-features branch from 90ecb16 to e133491 Compare October 19, 2020 14:51
@Kyle-Verhoog Kyle-Verhoog mentioned this pull request Oct 19, 2020
Kyle-Verhoog added a commit that referenced this pull request Oct 19, 2020
Versions with "rc" in them seem to register as spelling mistakes.

https://app.circleci.com/pipelines/github/DataDog/dd-trace-py/3057/workflows/ab9bf51f-d338-4bff-8549-08e02d4cbcf1/jobs/358186

@majorgreys introduced this fix to solve the issue in #1714.

Co-authored-by: Tahir H. Butt <[email protected]>
@Kyle-Verhoog Kyle-Verhoog merged commit 4933260 into master Oct 19, 2020
@Kyle-Verhoog Kyle-Verhoog deleted the Isabella-Pham/starlette-features branch October 19, 2020 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants