[WIP] flask: refactoring to build on opentelemetry wsgi #370
Conversation
Using the wsgi integration removes a significant amount of redundant code, and eliminates a bug where a copied flask context (which is an accepted scenario) exists after the span is actually closed. Fixes #349.
| attributes = otel_wsgi.collect_request_attributes(environ) | ||
| span = wsgi_tracer.get_current_span() | ||
| if flask_request.endpoint: | ||
| span.update_name(flask_request.endpoint) |
There was a problem hiding this comment.
The original point of not using the WSGI middleware was to avoid using update_name which is discouraged and indeed did not work at the time with the backend I used. For example, you can't configure sampling based on the endpoint, if you use update_name.
There was a problem hiding this comment.
Got it, that's good info. Sorry I missed that comment.
When you say it's use is discouraged, where is that stated? The specification states:
The Span's start and end timestamps reflect the elapsed real time of the operation. A Span's start time SHOULD be set to the current time on span creation. After the Span is created, it SHOULD be possible to change the its name, set its Attributes, and add Links and Events. These MUST NOT be changed after the Span's end time has been set.
Looking at the comment it's stated that this make it hard for dynatrace's span grouping. Have alternatives been tried that may be compatible with this, e.g. grouping on the exporter side?
There was a problem hiding this comment.
Here is the wording in the spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#updatename
It is highly discouraged to update the name of a Span after its creation. Span name is often used to group, filter and identify the logical groups of spans. And often, filtering logic will be implemented before the Span creation for performance reasons. Thus the name update may interfere with this logic.
For Dynatrace, this is no longer as important as it was at the time, but what the spec says still holds.
There was a problem hiding this comment.
got it. I'd argue against exposing update_name in the first place then, since the existence of that API means that people will probably use it, and would then require a processor to handle that case anyway.
I'm going to take a stab at seeing if I can start the span in the flask_before_request, and end it with the wsgi, which should avoid the use of update_name.
| @@ -76,6 +76,35 @@ def assert_environ(): | |||
| self.client.get("/assert_environ") | |||
| self.assertEqual(nonstring_keys, set()) | |||
|
|
|||
| def test_copy_current_request(self): | |||
There was a problem hiding this comment.
This is something that needs to be fixed though, and if we can't find another way then I'm even fine with using update_name.
Supporting B3's technical definition of a parentspanid, by sourcing the span id of the parent span during injection from the propagator.
Encode the span kind as a "span.kind" tag.
As discussed in the SIG call, move the alpha v4 release date three weeks back to account for ongoing work.
open-telemetry/opentelemetry-specification#370 added the requirement to have a "force_flush" method in the span processors. This commit exposes an already existing internal method on the batch span processor that does exactly the same, it also adds it to the span processor interface and as a no-op to the simple span processor.
Co-authored-by: Diego Hurtado <[email protected]> Co-authored-by: Christian Neumüller <[email protected]> Co-authored-by: Mauricio Vásquez <[email protected]>
open-telemetry#389 implemented force_flush() for the span processor. For BatchSpanProcessor it was implemented by exposing an already existing _flush() method, it created a race condition because the _flush() method was intended to be called only from the context of the worker thread, this because it uses the export() method that is not thread safe. The result after that PR is that some tests were failing randomly because export() was being executed in two different threads, the worker thread and the user thread calling force_flush(). This commit fixes it by implementing a more sophisticated flush mechanism. When a flush is requested, a special span token is inserted in the spans queue, a flag indicating a flush operation is on progress is set and the worker thread is waken up, after it a condition variable is monitored waiting for the worker thread to indicate that the token has been processed. The worker thread has a new logic to avoid sleeping (waiting on the condition variable) when there is a flush operation going on, it also notifies the caller (using another condition variable) when the token has been processed.
Initial implementation of the end-to-end metrics pipeline.
Co-authored-by: Diego Hurtado <[email protected]>
Co-authored-by: alrex <[email protected]>
This change implements the Context API portion of OTEP open-telemetry#66. The CorrelationContext API and Propagation API changes will come in future PRs. We're leveraging entrypoints to support other implementations of the Context API if/when necessary. For backwards compatibility, this change uses aiocontextvars for Python versions older than 3.7. Co-authored-by: Diego Hurtado <[email protected]> Co-authored-by: Mauricio Vásquez <[email protected]>
…ry#410) Following opentelemetry-specification/430.
The original method with wsgi used update_name, which is discouraged. Authoring a custom middleware to avoid that issue.
toumorokoshi
changed the title
toumorokoshi
changed the title
|
I'm relabeling this to a WIP, as, after further work, I'd like to explore making UpdateName a more acceptable option. At that point the WSGI extension option will be more blessed: |
|
Closing this for now, @toumorokoshi will reopen when he picks this back up. |
closes open-telemetry#370 Signed-off-by: Olivier Albertini <[email protected]>
Using the wsgi integration removes a significant amount
of redundant code, and eliminates a bug where a copied
flask context (which is an accepted scenario) exists
after the span is actually closed.
Fixes #349.