Skip to content

Conversation

@michaelyaakoby
Copy link
Member

Fixes #11 and more:

  • The option to "Exclude /pyctuator/**" was missing from "HTTP Traces" UI
  • Unneeded request logging in examples
  • Wrong content-type for pyctuator responses in "HTTP Traces" UI

@michaelyaakoby michaelyaakoby requested a review from yanoom June 1, 2020 08:30
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2020

Codecov Report

Merging #12 into master will increase coverage by 0.66%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   87.14%   87.80%   +0.66%     
==========================================
  Files          25       24       -1     
  Lines         879      853      -26     
==========================================
- Hits          766      749      -17     
+ Misses        113      104       -9     
Impacted Files Coverage Δ
pyctuator/impl/pyctuator_impl.py 89.61% <ø> (+4.99%) ⬆️
pyctuator/impl/__init__.py 100.00% <100.00%> (ø)
pyctuator/impl/fastapi_pyctuator.py 99.00% <100.00%> (+1.32%) ⬆️
pyctuator/impl/flask_pyctuator.py 97.95% <100.00%> (+1.48%) ⬆️
pyctuator/metrics/thread_metrics_impl.py 72.22% <0.00%> (+5.55%) ⬆️
pyctuator/metrics/memory_metrics_impl.py 56.52% <0.00%> (+8.69%) ⬆️

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 5cd6434...25e63d7. Read the comment docs.

@michaelyaakoby michaelyaakoby force-pushed the bugs branch 9 times, most recently from 922d25d to 665cec0 Compare June 2, 2020 14:36
@michaelyaakoby michaelyaakoby requested a review from MatanRubin June 2, 2020 14:36
@yanoom
Copy link
Contributor

yanoom commented Jun 3, 2020

Why did you decide to combile fastapi_http_tracer.py and flask_http_tracer.py into their flask/fast_api_pyctuator.py counterparts?

Copy link
Contributor

@yanoom yanoom left a comment

Choose a reason for hiding this comment

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

Please see my comments

@michaelyaakoby michaelyaakoby force-pushed the bugs branch 2 times, most recently from eae3187 to 9b1cfe6 Compare June 3, 2020 10:26
@michaelyaakoby
Copy link
Member Author

michaelyaakoby commented Jun 8, 2020

Why did you decide to combile fastapi_http_tracer.py and flask_http_tracer.py into their flask/fast_api_pyctuator.py counterparts?

The splitting to separate classes (they were classes) didn't create better abstractions, on the contrary, it forced us to either register two interceptors, one for recording request/response headers and another for injecting the content-type when needed, or mix the two.
As a side affect, the refactoring reduced the amount of boiler plate (class definition etc).

michael.yak added 3 commits June 10, 2020 13:41
In order for the "Http Traces" tab to be able to hide requests sent by
Spring Boot Admin to the Pyctuator endpoint, `pyctuator_endpoint_url`
must be using the same host and port as `app_url`.
The examples initially registered the host's IP for `app_url` instead
of using the same host as `pyctuator_endpoint_url` - the idea was that
the app's URL shown in SBA will be accessible.

Also, both FastAPI and Flask examples were logging every request sent
from SBA to Pyctuator. This was annoying so the example now are
configuring the loggers not to show access-log.
Responses sent by Pyctuator should set content-type to
`application/vnd.spring-boot.actuator.v2+json` - the existing mechanism
was invoked *after* requests were intercepted so the content-type was
missing tom "HTTP Traces" in SBA.
@MatanRubin MatanRubin merged commit 3fc7437 into master Jun 15, 2020
@MatanRubin MatanRubin deleted the bugs branch June 15, 2020 08:19
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.

http-trace: the "exclude /pyctuator/**" option is not available in UI

5 participants