Skip to content

fix aioredis span context handling#1462

Merged
basepi merged 3 commits into
elastic:mainfrom
zumalabs:fix-aioredis-context-handling
Feb 15, 2022
Merged

fix aioredis span context handling#1462
basepi merged 3 commits into
elastic:mainfrom
zumalabs:fix-aioredis-context-handling

Conversation

@qeternity

@qeternity qeternity commented Feb 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

ghost commented Feb 13, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

💚 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

beniwohli commented Feb 14, 2022

Copy link
Copy Markdown
Contributor

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

basepi
basepi requested changes Feb 14, 2022
View reviewed changes

@basepi basepi left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread elasticapm/instrumentation/packages/asyncio/aioredis.py Outdated
Comment thread elasticapm/instrumentation/packages/asyncio/aioredis.py Outdated
@qeternity
Merge pull request #1 from beniwohli/fix-aioredis-context-handling
c6132d5 
add test and make if clause more specific
@qeternity

qeternity commented Feb 14, 2022

Copy link
Copy Markdown
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

beniwohli commented Feb 15, 2022

Copy link
Copy Markdown
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

apm-agent-python/elasticapm/traces.py

Line 492 in ec9a733

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

qeternity commented Feb 15, 2022

Copy link
Copy Markdown
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

basepi commented Feb 15, 2022

Copy link
Copy Markdown
Contributor

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
@qeternity @beniwohli
fix aioredis span context handling (elastic#1462)
940f75c 
* fix aioredis span context handling

* add test and make if clause more specific

Co-authored-by: Benjamin Wohlwend <[email protected]>
@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

Reviewers

@basepi basepi basepi approved these changes

Assignees

No one assigned

Labels

agent-python community Issues opened by the community triage Issues awaiting triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@qeternity @beniwohli @basepi