search

SolarEdgeTech / pyctuator

Monitor Python applications using Spring Boot Admin
Apache License 2.0
175 stars 17 forks source link

create scrubber for request headers in trace #45

Closed mxab closed 3 years ago

mxab commented 3 years ago

PR for #44

This PR enables the HttpTracer to run scrubbing function similar to the environment scrubber

During the implementation I noticed that the different vendors differ. Some contain a list of values for each headers, some can only have one.

This PR normalises the traces to always contains a list.

Also flask has capitalised headers e.g.: Authorization vs authorization

Currently I'm checking this in the E2E but you could thinking about normaling it as well

MatanRubin commented 3 years ago

Thanks @mxab for the idea and PR! Looks great.

Does SBA itself do a similar scrubbing? If so, it might be a good idea to check what terms they're scrubbing.

mxab commented 3 years ago

Good point @MatanRubin , I think the SBA does nothing here since they just feed of the actuator endpoints.

But it looks like there are some restriction in spring boot's http traces implementation: https://github.com/spring-projects/spring-boot/blob/b4c9bb0d5c1df4b4788ebbede69e125e559870e2/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/trace/http/HttpExchangeTracer.java#L135

I guess it make sense to scrub also cookie values?

michaelyaakoby commented 3 years ago

@mxab the pipeline failed on typing and style checks done by mypy and pylint. See my comments with suggested fixes.

If you want to run the checks locally, you can use make check, though note that because some packages are optional its better to first install them using poetry -E psutils for example.

Again, thanks for the PR.

mxab commented 3 years ago

@michaelyaakoby I splitted up some code in the long lines during the scrubbing I created a new mapping object. One thing I noticed is that according to the typings the headers object will never be None therefore I removed the if headers block. But on the otherhand I think mypy is not seeing that tornado provides the headers in a different format (Mapping[str,str] ), maybe we should prevent this during the creation of the `TraceRecord´

@MatanRubin I scrub cookie headers now aswell

michaelyaakoby commented 3 years ago

Thanks @mxab , this is really awesome. What you've noticed with aiohttp's header-value being a string and not a list-of-strings is a bug IMO, opened #46 for this. Thanks very much, merging and will release a version in the coming days (i want to fix the aiohttp bug first).

mxab commented 3 years ago

Nice! happy to help 😊