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.

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

OpenTracing support #5188

Merged
merged 14 commits into from
Feb 3, 2020
Merged

OpenTracing support #5188

merged 14 commits into from
Feb 3, 2020

Conversation

tomaszszymanski129
Copy link
Member

@tomaszszymanski129 tomaszszymanski129 commented Jan 22, 2020

I want to merge this change because it resolves #4176

What is already implemented and working from whats pushed:

  • Tracing of DB queries using Django db instrumentation
  • Tracing of Django requests
  • Tracing of Graphql queries

What is left to do:

  • Decide what to trace in each of three above
  • Remove all 3rd party tracing libs (DataDog, Jaeger) as whichever we decide to use will be implemented on different ticket, this is just to set up OpenTracing.

NOTE: Example below is the one I got on DataDog during development. It might or might not be DD that we choose as the tracing provider (and this ticket is OpenTracing only).

image

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. Database migration files are up to date.
  6. The changes are tested.
  7. GraphQL schema and type definitions are up to date.
  8. Changes are mentioned in the changelog.

@tomaszszymanski129 tomaszszymanski129 self-assigned this Jan 22, 2020
@tomaszszymanski129 tomaszszymanski129 changed the title Opentracing graphene middleware Opentracing Jan 22, 2020
@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #5188 into master will increase coverage by 0.09%.
The diff coverage is 83.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5188      +/-   ##
==========================================
+ Coverage   91.68%   91.78%   +0.09%     
==========================================
  Files         271      272       +1     
  Lines       17547    17649     +102     
  Branches     1520     1530      +10     
==========================================
+ Hits        16088    16199     +111     
+ Misses       1063     1050      -13     
- Partials      396      400       +4
Impacted Files Coverage Δ
saleor/account/models.py 90.06% <ø> (ø) ⬆️
saleor/extensions/plugins/avatax/plugin.py 60.88% <ø> (ø) ⬆️
saleor/payment/gateways/razorpay/plugin.py 0% <ø> (ø) ⬆️
saleor/giftcard/models.py 100% <ø> (ø) ⬆️
saleor/account/validators.py 100% <ø> (ø) ⬆️
saleor/webhook/event_types.py 100% <ø> (ø) ⬆️
saleor/extensions/models.py 92.3% <ø> (ø) ⬆️
saleor/core/weight.py 100% <ø> (ø) ⬆️
saleor/checkout/models.py 95.45% <ø> (ø) ⬆️
saleor/payment/gateways/stripe/plugin.py 0% <ø> (ø) ⬆️
... and 78 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6486c6...2302502. Read the comment docs.

Copy link

django-queries commented Jan 22, 2020

Here is the report for 2302502 (mirumee/saleor @ feature/opentracing)
Base comparison is 997dbb5.

No differences were found. (click me)

# api.benchmark checkout
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  add billing address to checkout            	         34	         34	             20
  add shipping to checkout                   	          7	          7	              0
  checkout payment charge                    	         10	         10	              0
  complete checkout                          	          8	          8	              0
  create checkout                            	          5	          5	              1

# api.benchmark homepage
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  retrieve main menu                         	          5	          5	              0
  retrieve product list                      	          4	          4	              0
  retrieve secondary menu                    	          5	          5	              0
  retrieve shop                              	          2	          2	              0

# api.benchmark product
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  product details                            	         18	         18	              4
  retrieve product attributes                	          9	          9	              0

# api.benchmark variant
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  product variant bulk create                	         51	         51	              3
  retrieve variant list                      	         23	         23	              9

# api product sorting attributes
  test name                                  	left count 	right count	duplicate count
  -------------------------------------------	-----------	-----------	---------------
  sort product not having attribute data     	         21	         21	              0

@tomaszszymanski129 tomaszszymanski129 marked this pull request as ready for review January 30, 2020 16:26
@tomaszszymanski129 tomaszszymanski129 changed the title Opentracing OpenTracing support Jan 31, 2020
Copy link
Member

@NyanKiyoshi NyanKiyoshi left a comment

Choose a reason for hiding this comment

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

LGTM good work 👍

saleor/graphql/views.py Outdated Show resolved Hide resolved
saleor/settings.py Outdated Show resolved Hide resolved
saleor/graphql/views.py Outdated Show resolved Hide resolved
saleor/graphql/views.py Outdated Show resolved Hide resolved
saleor/graphql/views.py Outdated Show resolved Hide resolved
with opentracing.global_tracer().start_span(operation_name="query") as span:
span.set_tag("component", "db")
span.set_tag("db.statement", sql)
span.set_tag("db.many", many)
Copy link
Member

Choose a reason for hiding this comment

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

What is many and why do we need it?

Copy link
Member Author

Choose a reason for hiding this comment

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

That indicates whether the ultimately invoked call is django execute() or executemany(). Thought it might become handy.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's usually obvious from the query itself (LIMIT 1 etc.).

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, though defining a tag would allow to make it extra search criteria on external tracing platform. Well, on second thought it is unlikely somebody gonna ever make a use of it. Will drop that in a moment.

@maarcingebala maarcingebala merged commit c01e5a9 into master Feb 3, 2020
@maarcingebala maarcingebala deleted the feature/opentracing branch February 3, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OpenTracing
5 participants