Skip to content

Conversation

qeternity
Copy link
Contributor

@qeternity qeternity commented Feb 13, 2022

What does this pull request do?

Checks span context before attempting to mutate values.

A bit surprised this made it into a release...completely breaks code outside of instrumented paths.

@github-actions github-actions bot added agent-python community Issues opened by the community triage Issues awaiting triage labels Feb 13, 2022
@ghost
Copy link

ghost commented Feb 13, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-02-15T11:58:24.255+0000

  • Duration: 23 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 4811
Skipped 2930
Total 7741

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /test linters : Run the Python linters only.

  • /test full : Run the full matrix of tests.

  • /test benchmark : Run the APM Agent Python benchmarks tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@beniwohli
Copy link
Contributor

Hi @qeternity, good catch! I took the liberty to open a PR against your branch with a test, it's here: zumalabs#1

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

I think we want to set span.context if it doesn't exist.

add test and make if clause more specific
@qeternity
Copy link
Contributor Author

Hi @beniwohli @basepi - sure thing, just needed to get a working branch to triage some internal deps issues. Merging both of your suggestions.

@beniwohli
Copy link
Contributor

@qeternity @basepi actually, I don't think we should initialize context if it is None. It is only None for DroppedSpan instances (Span instances have context initialized to an empty dict, see

self.context = context if context is not None else {}
). For DroppedSpan instances, we discard the whole thing in the end, so we'd do unnecessary work here by initializing it.

@qeternity
Copy link
Contributor Author

@beniwohli this was my understanding as well from my initial read of the repo - I'll revert the last commit

@qeternity qeternity force-pushed the fix-aioredis-context-handling branch from 7146bfb to c6132d5 Compare February 15, 2022 08:59
@basepi
Copy link
Contributor

basepi commented Feb 15, 2022

Yep, my bad! Should have investigated more closely.

@basepi basepi merged commit d8788b2 into elastic:main Feb 15, 2022
qeternity added a commit to zumalabs/apm-agent-python that referenced this pull request Feb 20, 2022
* fix aioredis span context handling * add test and make if clause more specific Co-authored-by: Benjamin Wohlwend <beni@elastic.co>
@qeternity qeternity deleted the fix-aioredis-context-handling branch March 3, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-python community Issues opened by the community triage Issues awaiting triage
3 participants