Update BatchSpanProcessor to use new BatchProcessor class
#4580
Conversation
|
This should be all set to be merged |
…'s timeout which is bad, but the behavior before made even less sense..
opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py
Outdated
Show resolved
Hide resolved
| @@ -238,149 +236,6 @@ def test_on_start_accepts_parent_context(self): | |||
| span, parent_context=context | |||
| ) | |||
|
|
|||
| def test_shutdown(self): | |||
There was a problem hiding this comment.
Would it hurt to leave these tests which only test the public API of BatchSpanProcessor? It seems valuable that they continue passing as the implementation changes.
There was a problem hiding this comment.
I think these are all duplicates of the tests in shared_internal/test_batch_processor.py - that test suite does take the BatchSpanProcessor as a parameter and uses it in it's tests
There was a problem hiding this comment.
Ah got it thanks. Could you just add a comment mentioning that there are more tests there?
There was a problem hiding this comment.
I think these are all duplicates of the tests in
shared_internal/test_batch_processor.py- that test suite does take theBatchSpanProcessoras a parameter and uses it in it's tests
How hard would it be to create a base test class that can be included in each tests subdirectory to keep tests collocated with the implemenatations?
There was a problem hiding this comment.
I spent a couple hours trying to do this this morning and failed.
I tried switching the test class to sublass unittest.TestCase, and have the record_processor_class and telemetry as arguments to it's init.
But unittest's init is called on every test, it's not something I can call once to initialize/setup the test class.
With pytest I was using a decorator to inject those params, and it doesn't seem possible to decorate an imported test class.
If you have any ideas on how to implement let me know..
There was a problem hiding this comment.
Tried doing it with sublcassing for a while and also couldn't get it to work :(
Description
Update
BatchSpanProcessorto use newBatchProcessorclassType of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit tests
Does This PR Require a Contrib Repo Change?
Checklist: