You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/07/31 16:01:50 UTC
[GitHub] [skywalking-python] alonelaval opened a new pull request #55: Add CorrelationContext
alonelaval opened a new pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55
resolve apache/skywalking#5189
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #55: Add CorrelationContext
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55#discussion_r463918596
##########
File path: skywalking/trace/context/__init__.py
##########
@@ -118,23 +119,44 @@ def active_span(self):
return None
+ def get_correlation(self, key):
+ if key in self._correlation:
+ return self._correlation[key]
+ return None
+
+ def put_correlation(self, key, value):
+ if key is None or value is None:
Review comment:
If the `value is None`, I'm expecting the users want to remove the item, but you just ignore it
##########
File path: tests/plugin/sw_flask/services/provider.py
##########
@@ -24,12 +24,16 @@
config.logging_level = 'DEBUG'
agent.start()
- from flask import Flask, jsonify
+ from flask import Flask, jsonify, request
app = Flask(__name__)
@app.route("/users", methods=["POST", "GET"])
def application():
+ print(request.headers)
+ from skywalking.trace.context import get_context
+ print(get_context().get_correlation("test"))
+ print(get_context().get_correlation("test2"))
Review comment:
Do not simply print the correlation headers, this can not verify anything, even the headers are empty, the tests still passed, one way is to propagate these headers to the test codes, return the headers as the http response, for example, then get the response and verify the expected data here, `self.validate()` returns the response, and you can get the response and verify further
https://github.com/apache/skywalking-python/blob/0246634b8e908b845e1540c6d2b70d9058794601/tests/plugin/sw_flask/test_flask.py#L38
##########
File path: skywalking/trace/span/__init__.py
##########
@@ -225,5 +226,10 @@ class NoopSpan(Span):
def __init__(self, context: 'SpanContext' = None, kind: 'Kind' = None):
Span.__init__(self, context=context, kind=kind)
+ def extract(self, carrier: 'Carrier') -> 'Span':
+ if carrier is not None:
+ self.context._correlation = carrier.correlation_carrier.correlation
+
def inject(self, carrier: 'Carrier') -> 'Span':
+ carrier.correlation_carrier.correlation = self.context._correlation
Review comment:
What about making `_correlation` public or expose a property for it, instead of accessing a protected member directly?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] alonelaval commented on a change in pull request #55: Add CorrelationContext
Posted by GitBox <gi...@apache.org>.
alonelaval commented on a change in pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55#discussion_r463919800
##########
File path: skywalking/trace/span/__init__.py
##########
@@ -225,5 +226,10 @@ class NoopSpan(Span):
def __init__(self, context: 'SpanContext' = None, kind: 'Kind' = None):
Span.__init__(self, context=context, kind=kind)
+ def extract(self, carrier: 'Carrier') -> 'Span':
+ if carrier is not None:
+ self.context._correlation = carrier.correlation_carrier.correlation
+
def inject(self, carrier: 'Carrier') -> 'Span':
+ carrier.correlation_carrier.correlation = self.context._correlation
Review comment:
I want to protect context._correlation and let the user get or set values that need to be passed through through put and get
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] alonelaval commented on a change in pull request #55: Add CorrelationContext
Posted by GitBox <gi...@apache.org>.
alonelaval commented on a change in pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55#discussion_r463919895
##########
File path: skywalking/trace/context/__init__.py
##########
@@ -118,23 +119,44 @@ def active_span(self):
return None
+ def get_correlation(self, key):
+ if key in self._correlation:
+ return self._correlation[key]
+ return None
+
+ def put_correlation(self, key, value):
+ if key is None or value is None:
Review comment:
ok
##########
File path: tests/plugin/sw_flask/services/provider.py
##########
@@ -24,12 +24,16 @@
config.logging_level = 'DEBUG'
agent.start()
- from flask import Flask, jsonify
+ from flask import Flask, jsonify, request
app = Flask(__name__)
@app.route("/users", methods=["POST", "GET"])
def application():
+ print(request.headers)
+ from skywalking.trace.context import get_context
+ print(get_context().get_correlation("test"))
+ print(get_context().get_correlation("test2"))
Review comment:
ok
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #55: Add CorrelationContext
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55#discussion_r463918394
##########
File path: tests/plugin/sw_flask/services/provider.py
##########
@@ -24,12 +24,16 @@
config.logging_level = 'DEBUG'
agent.start()
- from flask import Flask, jsonify
+ from flask import Flask, jsonify, request
app = Flask(__name__)
@app.route("/users", methods=["POST", "GET"])
def application():
+ print(request.headers)
+ from skywalking.trace.context import get_context
+ print(get_context().get_correlation("test"))
+ print(get_context().get_correlation("test2"))
Review comment:
Do not simply print the correlation headers, this can not verify anything, even the headers are empty, the tests still passed, one way is to propagate these headers to the test codes, return the headers as the http response, for example, then get the response and verify the expected data here,
https://github.com/apache/skywalking-python/blob/0246634b8e908b845e1540c6d2b70d9058794601/tests/plugin/sw_flask/test_flask.py#L38
`self.validate()` returns the response, and you can get the response and verify further
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 merged pull request #55: Add CorrelationContext
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #55:
URL: https://github.com/apache/skywalking-python/pull/55
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org