dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

refactor: extract reusable logic from Agent Writer to HTTP Writer

Open jirikuncar opened this issue 3 years ago • 1 comments

This is preparing ground for CI Visibility Agentless support that will reuse retry logic from Agent Writer. Also we would like to keep the possibility for metrics calculation and submission in background.

https://github.com/DataDog/dd-trace-py/pull/3654#discussion_r866849329

Checklist

  • [ ] Added to the correct milestone.
  • [ ] Tests provided or description of manual testing performed is included in the code or PR.
  • [ ] Library documentation is updated.
  • [ ] Corp site documentation is updated (link to the PR).

jirikuncar avatar May 09 '22 10:05 jirikuncar

Codecov Report

Merging #3690 (38a8d8c) into 1.x (27aceb6) will decrease coverage by 0.11%. The diff coverage is 52.76%.

@@            Coverage Diff             @@
##              1.x    #3690      +/-   ##
==========================================
- Coverage   77.55%   77.44%   -0.12%     
==========================================
  Files         666      671       +5     
  Lines       51334    51566     +232     
==========================================
+ Hits        39811    39934     +123     
- Misses      11523    11632     +109     
Impacted Files Coverage Δ
ddtrace/contrib/pytest/constants.py 0.00% <0.00%> (ø)
ddtrace/contrib/pytest_bdd/__init__.py 0.00% <0.00%> (ø)
ddtrace/contrib/pytest_bdd/constants.py 0.00% <0.00%> (ø)
ddtrace/ext/test.py 0.00% <0.00%> (ø)
ddtrace/profiling/exporter/pprof_pb2.py 0.00% <0.00%> (ø)
ddtrace/profiling/exporter/pprof_pre319_pb2.py 0.00% <0.00%> (ø)
tests/contrib/grpc/hello_pb2.py 69.56% <50.00%> (-30.44%) :arrow_down:
ddtrace/contrib/pytest_bdd/plugin.py 53.33% <53.33%> (ø)
ddtrace/contrib/pytest/plugin.py 72.54% <75.00%> (-0.48%) :arrow_down:
ddtrace/internal/writer.py 84.08% <77.27%> (+0.17%) :arrow_up:
... and 2 more

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

codecov-commenter avatar May 09 '22 10:05 codecov-commenter

@Kyle-Verhoog I'm working on a fresh attempt at this change here, and I'm not sure I can see the need for the "has-a" relationship you described here. I think I've got a change that pretty well separates the concerns that are AgentWriter-specific from those that apply to the HTTPWriter using an "is-a" relationship. I'm not asking you to review that change, but could you provide any more detail on what "transmission state and corresponding metrics" would be better encapsulated in an instance attribute than a subclass relationship? Thanks a lot for loading this old comment back into your brain.

emmettbutler avatar Apr 04 '23 21:04 emmettbutler

@emmettbutler I think where I was coming from is the leaking of concerns between the base and subclasses. By doing an "is-a" relationship it is very easy to move a method up a layer of the classes and have the concerns leaked to the base.

I think "is-a" can work it's just much easier to fall into implicitly coupling the classes because it's so easy to share state between them.

Kyle-Verhoog avatar Apr 05 '23 19:04 Kyle-Verhoog