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

feat(botocore): patch stepfunctions #7514

Merged
merged 21 commits into from
Jan 12, 2024
Merged

feat(botocore): patch stepfunctions #7514

merged 21 commits into from
Jan 12, 2024

Conversation

agocs
Copy link
Contributor

@agocs agocs commented Nov 7, 2023 •
edited by emmettbutler
Loading

Patch botocore stepfunctions start-execution and start-sync-execution. We want to be able to add trace context to the input so the stepfunction logs trace reducer can link stepfunction traces to upstream traces.

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
  • 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.
  • This PR doesn't touch any of that.

@agocs agocs requested review from a team as code owners November 7, 2023 19:11
@majorgreys
Copy link
Contributor

@agocs Can you add a release note for this feature change?

https://ddtrace.readthedocs.io/en/stable/releasenotes.html#generating-a-release-note

@majorgreys majorgreys changed the title [SVLS-4126] Patch botocore stepfunctions feat(botocore): patch stepfunctions Nov 8, 2023
@pr-commenter
Copy link

pr-commenter bot commented Nov 13, 2023 •
edited
Loading

Benchmarks

Benchmark execution time: 2024-01-12 00:00:30

Comparing candidate commit 186bf7f in PR branch chris.agocs/patch_botocore_stepfunctions with baseline commit e6c5242 in branch main.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 194 metrics, 9 unstable metrics.

scenario:flasksimple-appsec-get

  • 🟩 max_rss_usage [-910.784KB; -732.531KB] or [-2.514%; -2.022%]

@agocs agocs requested a review from a team as a code owner November 13, 2023 22:46
@majorgreys majorgreys changed the base branch from 2.x to main November 30, 2023 18:58
ZStriker19
ZStriker19 approved these changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@ZStriker19 ZStriker19 left a comment

Choose a reason for hiding this comment

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

Is extraction already implemented?

@ZStriker19
Copy link
Contributor

Is extraction already implemented?

Oh or there is no extraction? Because "stepfunction logs trace reducer can link stepfunction traces to upstream traces."

@agocs
Copy link
Contributor Author

agocs commented Dec 4, 2023

@ZStriker19 That's correct, extraction will be implemented in the logs-to-traces reducer

@agocs
Merge branch 'chris.agocs/patch_botocore_stepfunctions' of github.com…
5701c9c 
…:DataDog/dd-trace-py into chris.agocs/patch_botocore_stepfunctions
@emmettbutler emmettbutler enabled auto-merge (squash) December 5, 2023 17:56
@emmettbutler emmettbutler marked this pull request as draft December 5, 2023 18:24
auto-merge was automatically disabled December 5, 2023 18:24

Pull request was converted to draft

purple4reina
purple4reina requested changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@purple4reina purple4reina left a comment

Choose a reason for hiding this comment

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

Let's do a ducktype check before adding the dd header dict.

Also, looks like we could use more test coverage of this inject_trace_to_stepfunction_input. I'm seeing 5 different code paths, yet only one test.

@agocs agocs requested a review from purple4reina December 11, 2023 20:21
@agocs agocs marked this pull request as ready for review December 11, 2023 20:23
@agocs
Copy link
Contributor Author

agocs commented Dec 14, 2023

I think I'm going to wait until after the code freeze to merge this rather than rush to get it in before EOY.

@agocs agocs merged commit f7fa041 into main Jan 12, 2024
50 of 54 checks passed
@agocs agocs deleted the chris.agocs/patch_botocore_stepfunctions branch January 12, 2024 18:01
@agocs agocs mentioned this pull request Aug 16, 2024
Merged
2 tasks
agocs added a commit that referenced this pull request Aug 28, 2024
@agocs
fix(botocore): inject trace context into stepfunction start_execution…
c2822c5 
… input (#10262)

In #7514 , I added a feature to patch start_execution calls to AWS
stepfunctions and inject trace context into the input object. That
feature suffered a regression in #8937 , and the whole stepfunctions
integration was refactored to use the Core API in #9005 . This PR
re-adds the injection of trace context into the start_execution input,
and adds a few unit tests to detect the same regression in the future.


## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
@DylanLovesCoffee DylanLovesCoffee mentioned this pull request Sep 9, 2024
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZStriker19 ZStriker19 ZStriker19 approved these changes

@purple4reina purple4reina purple4reina approved these changes

@emmettbutler emmettbutler emmettbutler approved these changes

@tabgok tabgok Awaiting requested review from tabgok tabgok was automatically assigned from DataDog/apm-framework-integrations-reviewers-py

@majorgreys majorgreys Awaiting requested review from majorgreys

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agocs @majorgreys @ZStriker19 @emmettbutler @purple4reina