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.

Sign up for GitHub

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

Jump to bottom

chore(internal): core api #6202

Merged
merged 10 commits into from
Jul 7, 2023
Merged

chore(internal): core api #6202

merged 10 commits into from
Jul 7, 2023

Conversation

emmettbutler
Copy link
Collaborator

@emmettbutler emmettbutler commented Jun 23, 2023 •
edited

This pull request implements the "Core API", providing an interface to the creation of context trees with which product code can store data. This API will be used in the future as the main interface between products and contrib integrations.

To enable compatibility with existing tests and usage patterns in which contexts are assumed to have the same lifetime as certain spans, this implementation of the Core API allows ExecutionContext instances to be attached to spans. Building the API this way allows incremental changes to be merged without requiring hundreds of tests to change their code. Planned future changes will break the temporal dependency between spans and contexts.

In this change, the Core API isn't used anywhere. #6200 will replace ddtrace.internal._context with this API.

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@emmettbutler emmettbutler requested a review from a team as a code owner June 23, 2023 17:11
@emmettbutler emmettbutler marked this pull request as draft June 23, 2023 17:11
@emmettbutler emmettbutler added the changelog/no-changelog A changelog entry is not required for this PR. label Jun 23, 2023
@emmettbutler
ignore type on span._execution_context because it does not exist yet
240bbf8 
since this API is not used anywhere, this attribute access will not fail
@emmettbutler emmettbutler marked this pull request as ready for review June 23, 2023 17:32
@pr-commenter
Copy link

pr-commenter bot commented Jun 23, 2023 •
edited

Benchmarks

Comparing candidate commit 0289cff in PR branch emmett.butler/context-events-api with baseline commit 3053460 in branch 1.x.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 94 cases.

@emmettbutler emmettbutler mentioned this pull request Jun 26, 2023
Merged
16 tasks
@emmettbutler emmettbutler removed the request for review from brettlangdon June 28, 2023 18:08
@emmettbutler emmettbutler enabled auto-merge (squash) July 6, 2023 16:03
@emmettbutler emmettbutler merged commit 8c74ecd into 1.x Jul 7, 2023
116 of 121 checks passed
@emmettbutler emmettbutler deleted the emmett.butler/context-events-api branch July 7, 2023 14:40
@github-actions github-actions bot added this to the v1.17.0 milestone Jul 7, 2023
emmettbutler added a commit that referenced this pull request Jul 12, 2023
@emmettbutler @gnufede @ZStriker19
chore(tracing): use the core api (#6200)
0ca3703 
This pull request replaces `ddtrace.internal._context` with
`ddtrace.internal.core`, which was added in
#6202.

In an upcoming pull request, I plan to entirely remove the need for the
setters and getters in `core` to accept an optional `span` argument, so
you can think of that argument as a stopgap that enables incremental
change.

There are a few minor test changes related to where context data lives
relative to spans, and the `_appsec_request_context` is changed
significantly to use the core API.

Existing tests are enough to cover this change, because there's no new
functionality introduced.

## Changes

* Replaces all uses of `ddtrace.internal._context` with uses of
`ddtrace.internal.core`, a new module with more functionality
* Updates `_asm_request_context`'s context management backend to use the
core API as opposed to a bare `ContextVar`
* Changes `ddtrace.contrib.subprocess.patch` to store contextual data
without spans, because cross-process context parenting is simpler
without spans
* Updates `ddtrace.opentelemetry._span` to use the core API
* Updates `test_ctx_api` to make explicit its dependence on span
lifetimes for context propagation
* Removes `ddtrace.internal._context`

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](../docs/contributing.rst#release-branch-maintenance))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](../docs/contributing.rst#release-branch-maintenance)

---------

Co-authored-by: Federico Mon <federico.mon@datadoghq.com>
Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com>
romainkomorndatadog pushed a commit that referenced this pull request Aug 8, 2023
@emmettbutler @romainkomorndatadog
chore(internal): core api (#6202)
a8f75d2 
This pull request implements the "Core API", providing an interface to
the creation of context trees with which product code can store data.
This API will be used in the future as the main interface between
products and contrib integrations.

To enable compatibility with existing tests and usage patterns in which
contexts are assumed to have the same lifetime as certain spans, this
implementation of the Core API allows `ExecutionContext` instances to be
attached to spans. Building the API this way allows incremental changes
to be merged without requiring hundreds of tests to change their code.
Planned future changes will break the temporal dependency between spans
and contexts.

In this change, the Core API isn't used anywhere.
#6200 will replace
`ddtrace.internal._context` with this API.
romainkomorndatadog pushed a commit that referenced this pull request Aug 8, 2023
@emmettbutler @gnufede
@ZStriker19 @romainkomorndatadog
chore(tracing): use the core api (#6200)
752f895 
This pull request replaces `ddtrace.internal._context` with
`ddtrace.internal.core`, which was added in
#6202.

In an upcoming pull request, I plan to entirely remove the need for the
setters and getters in `core` to accept an optional `span` argument, so
you can think of that argument as a stopgap that enables incremental
change.

There are a few minor test changes related to where context data lives
relative to spans, and the `_appsec_request_context` is changed
significantly to use the core API.

Existing tests are enough to cover this change, because there's no new
functionality introduced.

## Changes

* Replaces all uses of `ddtrace.internal._context` with uses of
`ddtrace.internal.core`, a new module with more functionality
* Updates `_asm_request_context`'s context management backend to use the
core API as opposed to a bare `ContextVar`
* Changes `ddtrace.contrib.subprocess.patch` to store contextual data
without spans, because cross-process context parenting is simpler
without spans
* Updates `ddtrace.opentelemetry._span` to use the core API
* Updates `test_ctx_api` to make explicit its dependence on span
lifetimes for context propagation
* Removes `ddtrace.internal._context`

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](../docs/contributing.rst#release-branch-maintenance))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](../docs/contributing.rst#release-branch-maintenance)

---------

Co-authored-by: Federico Mon <federico.mon@datadoghq.com>
Co-authored-by: Zachary Groves <32471391+ZStriker19@users.noreply.github.com>
@emmettbutler emmettbutler mentioned this pull request Dec 19, 2023
Merged
18 tasks
majorgreys pushed a commit that referenced this pull request Jan 3, 2024
@emmettbutler
chore: deprecate the span= arguments on ExecutionContext methods (#7957)
14c0a97 
This pull request deprecates the `span` arguments on `ExecutionContext`
methods to indicate that they are only for backward compatibility and
not meant to be used by newly written code. This has been the plan since
the beginning of the core API in
#6202 (comment).

Deprecations are usually used for non-internal functionality, but I
think a deprecation is appropriate in this internal case as we encourage
broader usage of the core API.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.
ZStriker19 pushed a commit to haozhun/dd-trace-py that referenced this pull request Jan 8, 2024
@emmettbutler @ZStriker19
chore: deprecate the span= arguments on ExecutionContext methods (Dat…
51eb2ce 
…aDog#7957)

This pull request deprecates the `span` arguments on `ExecutionContext`
methods to indicate that they are only for backward compatibility and
not meant to be used by newly written code. This has been the plan since
the beginning of the core API in
DataDog#6202 (comment).

Deprecations are usually used for non-internal functionality, but I
think a deprecation is appropriate in this internal case as we encourage
broader usage of the core API.

## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabgok tabgok tabgok approved these changes

@mabdinur mabdinur mabdinur approved these changes

@majorgreys majorgreys Awaiting requested review from majorgreys majorgreys is a code owner automatically assigned from DataDog/apm-core-python

@juanjux juanjux Awaiting requested review from juanjux

@P403n1x87 P403n1x87 Awaiting requested review from P403n1x87

@Kyle-Verhoog Kyle-Verhoog Awaiting requested review from Kyle-Verhoog

@christophe-papazian christophe-papazian Awaiting requested review from christophe-papazian

Assignees
No one assigned
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Milestone
v1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@emmettbutler @tabgok @mabdinur