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

Conditionally create server spans for tornado and refactoring other frameworks to use utility function #889

Merged
merged 2 commits into from
Feb 3, 2022
Merged

Conditionally create server spans for tornado and refactoring other frameworks to use utility function #889

merged 2 commits into from
Feb 3, 2022

Conversation

ashu658
Copy link
Member

@ashu658 ashu658 commented Feb 1, 2022 •
edited
Loading

Description

  • Adds support to make spans as INTERNAL if a span is already present in current context in tornado.
  • Refactoring code in Django, Pyramid, Flask to use the utility funciton i.e. _start_internal_or_server_span

Fixes #450

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
    Currently falcon only makes SERVER spans. With this PR it can make INTERNAL spans in presence of a span in current context.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@ashu658 ashu658 requested a review from a team February 1, 2022 07:58
@ashu658 ashu658 changed the title Conditionally create server spans for tornado [IN-PROGRESS] Conditionally create server spans for tornado Feb 1, 2022
@ashu658 ashu658 changed the title [IN-PROGRESS] Conditionally create server spans for tornado [WIP] Conditionally create server spans for tornado Feb 1, 2022
@ashu658 ashu658 changed the title [WIP] Conditionally create server spans for tornado Conditionally create server spans for tornado Feb 2, 2022
@owais
Copy link
Contributor

owais commented Feb 2, 2022 •
edited
Loading

This contains changes to Django, Pyramid and Flask but PR claims to only add support for Tornado. I'd suggest to split out any refactor to another PR.

The changes are small enough and LGTM but please update PR title/description and git commit message to reflect all work done in this branch.

@owais owais enabled auto-merge (squash) February 2, 2022 12:20
@owais owais disabled auto-merge February 2, 2022 12:21
@ashu658 ashu658 changed the title Conditionally create server spans for tornado Conditionally create server spans for tornado and refactoring Django, Pyramid and Flask to use utility function Feb 2, 2022
@ashu658 ashu658 changed the title Conditionally create server spans for tornado and refactoring Django, Pyramid and Flask to use utility function Conditionally create server spans for tornado and refactoring other frameworks to use utility function Feb 2, 2022
Mark span as internal in presence of another span for tornado and use
e05a113 
_start_internal_or_server_span function for flask, pyramid and django

Adding changelog entry

Adding unit test and fixing lint errors

Refactoring to use _start_internal_or_server_span function

Removing unwanted imports and variables

Fixing lint errors

adding changes from tox -e generate

Fixing build errors
@ashu658 ashu658 force-pushed the conditional-server-span-tornado branch from 17b9f92 to e05a113 Compare February 2, 2022 13:49
@ashu658
Copy link
Member Author

ashu658 commented Feb 2, 2022

@owais Can you help me with the failing 'Combine benchmarks from previous build job'? I am not sure how to fix this.

@ashu658 ashu658 requested a review from owais February 2, 2022 14:30
@owais
Copy link
Contributor

owais commented Feb 2, 2022

Just re-run it and hope it passes next time :) If it keeps failing consistently then we need to identify the performane degradation and fix it or update the perf numbers

@ashu658
Copy link
Member Author

ashu658 commented Feb 3, 2022 •
edited
Loading

I guess I don't have the access to re-run tests. Can you re-run the test @owais ?
Tests passed after merge commit but how can I re-run these tests in case I need to do this later?

@owais owais merged commit 0e92d6a into open-telemetry:main Feb 3, 2022
@ashu658 ashu658 deleted the conditional-server-span-tornado branch February 3, 2022 08:52
@ashu658 ashu658 mentioned this pull request Mar 8, 2022
Closed
6 tasks
@ashu658 ashu658 mentioned this pull request Apr 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owais owais Awaiting requested review from owais

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

Successfully merging this pull request may close these issues.

Tornado: Conditionally create SERVER spans
2 participants
@ashu658 @owais